Skip to content

fix(openai): generically handle models that reject unsupported request params - #1852

Open
arimu1 wants to merge 7 commits into
embabel:mainfrom
arimu1:fix/1724-unsupported-request-params
Open

fix(openai): generically handle models that reject unsupported request params#1852
arimu1 wants to merge 7 commits into
embabel:mainfrom
arimu1:fix/1724-unsupported-request-params

Conversation

@arimu1

@arimu1 arimu1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1724 — some OpenAI models (GPT-5 family, and now GPT-4.1) return 400 when non-default sampling parameters such as temperature are sent.

This PR implements both strategies from the issue:

  1. Declarative capabilities (primary)

    • Introduces ModelCapabilities + CapabilityAwareOpenAiOptionsConverter that omits unsupported parameters (temperature, topP, frequency/presence penalty).
    • Replaces the Gpt5ChatOptionsConverter / StandardOpenAiOptionsConverter branch in OpenAiModelsConfig with a single capability-aware converter.
    • Expands SpecialHandlingConfiguration (YAML special_handling) with optional flags for topP and penalties.
    • Marks the gpt-4.1 family with supports_temperature: false (GPT-5 family already marked).
    • Keeps Gpt5ChatOptionsConverter / StandardOpenAiOptionsConverter as thin aliases for compatibility.
  2. Defensive retry (safety net)

    • InstrumentedChatModel catches provider errors matching Unsupported value: '<param>', strips that parameter from chat options, logs a warning, and retries once.
    • Covers uncatalogued restricted models and future provider changes.
  3. Message-sender fix

    • SpringAiLlmMessageSender fallback tool-options path only copies non-null parameters so intentionally omitted values (e.g. temperature) are not re-introduced.

Test plan

  • CapabilityAwareOpenAiOptionsConverterTest — default / temperature-restricted / multi-param restricted / aliases
  • Existing Gpt5ChatOptionsConverterTest + StandardOpenAiOptionsConverterTest
  • UnsupportedRequestParameterHandlerTest — parse OpenAI error + strip params
  • InstrumentedChatModelTest — retry on unsupported temperature; no retry for unrelated errors
  • SpringAiLlmMessageSenderTest — fallback does not re-add omitted temperature when attaching tools
  • OpenAiModelLoaderTest — GPT-4.1 models load with supports_temperature: false
export JAVA_HOME=$(/usr/libexec/java_home -v 21)
mvn -pl embabel-agent-openai,embabel-agent-api,embabel-agent-autoconfigure/models/embabel-agent-openai-autoconfigure -am \
  -Dmaven.gitcommitid.skip=true \
  -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest=CapabilityAwareOpenAiOptionsConverterTest,Gpt5ChatOptionsConverterTest,StandardOpenAiOptionsConverterTest,UnsupportedRequestParameterHandlerTest,InstrumentedChatModelTest,SpringAiLlmMessageSenderTest,OpenAiModelLoaderTest \
  test

JDK 21 — all green.

Notes

  • AI-assisted implementation (Grok), reviewed and tested locally.
  • DCO signed-off-by on commit.

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arimu1 - thank you for taking this important task!
Please see comments.
Btw., nice picture, on the bank of Bund river in Shangai? :)

arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 2, 2026
Address @igordayen review on embabel#1852:

- Rename UnsupportedRequestParameterHandler → UnsupportedRequestParameterRetry
  (avoid "Handler" event/callback connotation)
- Expand KDoc: not a structured provider API; OpenAI/Azure message pattern;
  fail closed; strip when() maintenance notes; normalize is not Spring binder
- Clarify Prompt.options is required for strip retry (else rethrow)
- Rename SpecialHandlingConfiguration → SupportFeaturesConfiguration
  (YAML special_handling property name unchanged)

Tests: UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest,
CapabilityAwareOpenAiOptionsConverterTest, OpenAiModelLoaderTest green (JDK 21)

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1

arimu1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Thanks for the thorough review — addressed on tip 69c9bd1fa:

Naming

  • HandlerUnsupportedRequestParameterRetry — avoids the event/callback connotation; object is only parse+strip for a one-shot retry.
  • SpecialHandlingConfigurationSupportFeaturesConfiguration — clearer “which sampling params does this model support?” YAML key/property stays special_handling / specialHandling for compatibility.

Reliability of message parse (is there an API?)

Not guaranteed across providers. Spring AI / provider clients typically surface the rejection as exception message text only. OpenAI’s JSON body can include error.param, but that field is not available on the ChatModel.call throwable path we get. There is no portable cross-provider “unsupported parameter” API. We match the well-known OpenAI/Azure wording from #1724 and fail closed (rethrow) when the pattern does not match or the param is unknown. Prefer declarative omit via YAML / ModelCapabilities; this retry is a safety net when flags are incomplete.

Maintaining the when strip map

Only ChatOptions sampling mutators we know how to clear. Adding a new strip-able option = add a branch + unit test. Unknown names return null (no invented options).

normalize vs Spring relaxed binder

Same spirit (snake_case / camelCase) but not Spring’s binder — we parse free-form exception text (wire names like top_p), not configuration properties. Documented in KDoc.

Are options mandatory?

Yes for recovery: without Prompt.options there is nothing to strip, so we rethrow the original error. Same if parse/strip returns null.

Tests re-run green (JDK 21): UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest, CapabilityAwareOpenAiOptionsConverterTest, OpenAiModelLoaderTest.

@igordayen

Copy link
Copy Markdown
Contributor

Not guaranteed across providers. Spring AI / provider clients typically surface the rejection as exception message text only. OpenAI’s JSON body can include error.param, but that field is not available on the ChatModel.call throwable path we get. There is no portable cross-provider “unsupported parameter” API. We match the well-known OpenAI/Azure wording from #1724 and fail closed (rethrow) when the pattern does not match or the param is unknown. Prefer declarative omit via YAML / ModelCapabilities; this retry is a safety net when flags are incomplete.

@arimu1 - yes, that is a concern.
If OpenAI / Spring changes the error wording, the condition will not hold. Wording got captured in tests:

                "400 Unsupported value: 'temperature' does not support 0.8 with this model. Only the default (1) value is supported."

What about non-OpenAI providers?
But it's really very limited testing.
@alexheifetz - should the LLM database be the source of truth?

@igordayen

Copy link
Copy Markdown
Contributor

From Claude:
Good news — you don't need reflection or internal API access. Looking at how Spring AI actually constructs NonTransientAiException, the message is the raw JSON body from OpenAI, just prefixed with the HTTP status:

org.springframework.ai.retry.NonTransientAiException: 400 - { "error": { "message": "Unsupported value: 'temperature' does not support 0.8 with this model. Only the default (1) value is supported.", "type": "invalid_request_error", "param": "temperature", "code": "unsupported_value" } }

This is confirmed by other Spring AI issues where the exception message is literally "400 - " followed by the complete error JSON, including type, param, and code fields — e.g. "400 - { "error": { "message": "Unknown parameter: 'response_format.schema'.", "type": "invalid_request_error", "param": "response_format.schema", "code": "unknown_parameter" } }". So handleError in SpringAiRetryAutoConfiguration is passing the whole response body through as the message — you don't need to dig into a cause chain or a different exception field.

That means extraction is just: strip the "NNN - " prefix, then parse the remainder as JSON.

private Optional<OpenAiErrorDetail> extractErrorDetail(NonTransientAiException e) {
    String message = e.getMessage();
    int jsonStart = message.indexOf('{');
    if (jsonStart < 0) {
        return Optional.empty(); // not a structured OpenAI error body
    }
    try {
        JsonNode root = objectMapper.readTree(message.substring(jsonStart));
        JsonNode error = root.path("error");
        if (error.isMissingNode()) {
            return Optional.empty();
        }
        return Optional.of(new OpenAiErrorDetail(
            error.path("code").asText(null),
            error.path("param").asText(null),
            error.path("type").asText(null)
        ));
    } catch (JsonProcessingException parseFailure) {
        log.debug("Error body wasn't valid JSON, treating as unstructured", parseFailure);
        return Optional.empty();
    }
}

Then your retry gate becomes:

Optional<OpenAiErrorDetail> detail = extractErrorDetail(e);
if (detail.isPresent() && "unsupported_value".equals(detail.get().code()) && detail.get().param() != null) {
    // strip detail.get().param() and retry once
} else {
    throw e; // fail closed on anything unstructured
}

Two caveats worth flagging:

  1. jsonStart = indexOf('{') is a bit blunt — it works because the message format is always "<status> - <json>", but if a future Spring AI version changes that prefix shape, this still degrades safely (JSON parse fails → Optional.empty() → fail closed, not a silent wrong retry).
  2. Non-OpenAI providers won't have this shape at all — Anthropic, Bedrock, etc. wrap errors differently. If you're multi-provider, keep this extractor OpenAI-specific and gate it on the model's provider, rather than assuming every NonTransientAiException has an error.code/error.param payload.

This is strictly better than the earlier regex approach: you're parsing the actual structured error OpenAI sent, just recovered from where Spring AI happens to have stashed it, rather than pattern-matching English prose.

@igordayen

Copy link
Copy Markdown
Contributor

from Claude - even more:

Here's a config-driven version:

restricted-model-options.yaml (e.g. src/main/resources/)

# Models that reject specific ChatOptions parameters, or accept them
# only at a fixed value. Source: OpenAI docs / observed 400s.
# Update this file when a new restriction is discovered — no redeploy
# should require touching Java code.
restrictedModels:
  gpt-5:
    unsupportedParams: [temperature, top_p]
    lastVerified: "2026-07-01"
    notes: "Only default temperature (1) accepted"

  gpt-5-mini:
    unsupportedParams: [temperature, top_p]
    lastVerified: "2026-07-01"

  o3-mini:
    unsupportedParams: [temperature, top_p, presence_penalty, frequency_penalty]
    lastVerified: "2026-06-15"

  o1:
    unsupportedParams: [temperature, top_p, presence_penalty, frequency_penalty]
    lastVerified: "2026-05-20"
    notes: "logprobs also unsupported but not a ChatOptions field today"

Loader + converter

public record RestrictedModelSpec(String model, Set<String> unsupportedParams) {}

@Component
public class RestrictedModelRegistry {

    private final Map<String, Set<String>> restrictions;

    public RestrictedModelRegistry(
            @Value("classpath:restricted-model-options.yaml") Resource resource,
            ObjectMapper yamlMapper) throws IOException {

        JsonNode root = yamlMapper.readTree(resource.getInputStream());
        JsonNode models = root.path("restrictedModels");

        Map<String, Set<String>> parsed = new HashMap<>();
        models.fields().forEachRemaining(entry -> {
            String modelName = entry.getKey();
            Set<String> params = new HashSet<>();
            entry.getValue().path("unsupportedParams")
                 .forEach(p -> params.add(p.asText()));
            parsed.put(modelName, Set.copyOf(params));
        });

        this.restrictions = Map.copyOf(parsed);
    }

    public Set<String> unsupportedParamsFor(String model) {
        return restrictions.getOrDefault(model, Set.of());
    }

    /** For the Layer-2 "learn once, remember" cache to feed back into. */
    public void recordDiscovered(String model, String param) {
        // if you want runtime learning to persist across restarts,
        // write this to a side file/DB rather than mutating `restrictions`
        // (which is intentionally immutable — see below)
    }
}

Note restrictions is built once and made immutable (Map.copyOf) — that's deliberate: config reload should be an explicit, restart-triggered event, not something mutated silently at runtime by request threads. If you want live reload without a restart, wrap it in a RefreshScope bean or watch the file with Spring Cloud Config instead of hand-rolling a poller.

The converter, now table-driven instead of hardcoded

@Component
public class RestrictedModelOptionsConverter implements OptionsConverter {

    private final RestrictedModelRegistry registry;

    RestrictedModelOptionsConverter(RestrictedModelRegistry registry) {
        this.registry = registry;
    }

    @Override
    public ChatOptions convert(String model, ChatOptions options) {
        Set<String> restricted = registry.unsupportedParamsFor(model);
        if (restricted.isEmpty()) return options;

        var mutated = options.mutate();
        if (restricted.contains("temperature") && options.getTemperature() != null) {
            mutated.temperature(null);
            log.warn("Dropped temperature for restricted model {}", model);
        }
        if (restricted.contains("top_p") && options.getTopP() != null) {
            mutated.topP(null);
            log.warn("Dropped top_p for restricted model {}", model);
        }
        // extend per param — a small reflective/functional map from
        // param-name -> BiConsumer<Mutator, Options> avoids this if/else
        // ladder growing unboundedly; worth doing once you're past ~5 params
        return mutated.build();
    }
}

One test to keep this honest

@Test
void yamlLoadsAndMatchesKnownRestrictedModel() {
    var registry = new RestrictedModelRegistry(
        new ClassPathResource("restricted-model-options.yaml"), yamlMapper);

    assertThat(registry.unsupportedParamsFor("gpt-5"))
        .containsExactlyInAnyOrder("temperature", "top_p");
    assertThat(registry.unsupportedParamsFor("gpt-4o"))
        .isEmpty(); // unrestricted model, sanity check
}

Two things worth deciding before this goes in:

  1. lastVerified/notes are currently decorative — if you want the "stale table" risk (mentioned earlier) to actually surface, you could add a scheduled job that logs a warning for any entry with lastVerified older than, say, 90 days, nudging someone to re-check it against OpenAI's docs.
  2. Where does Layer 2's runtime-discovered restriction go? Right now recordDiscovered is a no-op stub — decide if it should (a) just live in an in-memory cache for the life of the JVM, (b) get written back to the YAML file (risky — file becomes a build artifact and a runtime-mutated file at once), or (c) go to a small DB table separate from the static config. (c) is usually cleanest: static config = curated/reviewed, dynamic table = "things we caught live," and you periodically promote entries from the dynamic table into the reviewed YAML.

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arimu1 - thanks for contributing. attached quick resarch from from Claude. Looking for more flexible , framework-centric solution.
Also - merge conflicts.
Thank you.

arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 3, 2026
Address @igordayen review on embabel#1852:

- Rename UnsupportedRequestParameterHandler → UnsupportedRequestParameterRetry
  (avoid "Handler" event/callback connotation)
- Expand KDoc: not a structured provider API; OpenAI/Azure message pattern;
  fail closed; strip when() maintenance notes; normalize is not Spring binder
- Clarify Prompt.options is required for strip retry (else rethrow)
- Rename SpecialHandlingConfiguration → SupportFeaturesConfiguration
  (YAML special_handling property name unchanged)

Tests: UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest,
CapabilityAwareOpenAiOptionsConverterTest, OpenAiModelLoaderTest green (JDK 21)

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 3, 2026
…of truth

Rebased on main (OptionsConverter 2-arg + model stamp). Prefer OpenAI JSON
error.param from Spring AI exception messages over English wording for the
one-shot retry safety net. Declarative omit via openai-models.yml
special_handling → ModelCapabilities remains the primary path (LLM model
database). Fail closed on non-retryable error codes.

Addresses igordayen design feedback on embabel#1852 / embabel#1724.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1
arimu1 force-pushed the fix/1724-unsupported-request-params branch from 69c9bd1 to 019aad8 Compare August 3, 2026 01:09
@arimu1

arimu1 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Thanks — agreed the wording-only retry was the weak layer. Pushed tip 019aad8e2 (rebased on current main, including OptionsConverter(options, model)).

Design stance (aligned with your / Claude notes)

1. LLM model database is the source of truth (primary path)
We deliberately did not add a second restricted-model-options.yaml. OpenAI restrictions live in the existing model registry:

  • openai-models.ymlspecial_handling.supports_temperature: false (etc.)
  • SupportFeaturesConfigurationModelCapabilities
  • CapabilityAwareOpenAiOptionsConverter omits params before the call

That is the same role as the suggested restricted-model registry, but it stays inside the existing YAML model DB so maintainers update one place. GPT-4.1 family already has supports_temperature: false there (issue #1724).

2. Retry is only a safety net
When a capability flag is missing/incomplete, InstrumentedChatModel may strip one param and retry once. Fail closed otherwise.

3. Extraction no longer depends on English prose first
Spring AI surfaces the OpenAI body as
"400 - { \"error\": { \"param\": \"temperature\", \"code\": \"unsupported_value\", ... } }".

We now:

  1. Prefer structured JSON error.param (and only retry for known codes like unsupported_value / unknown_parameter / invalid_value, or missing code + present param)
  2. Fall back to the old Unsupported value: '…' regex for unstructured messages
  3. Return null → rethrow for unrelated codes (e.g. context_length_exceeded)

So the retry gate matches the structured field OpenAI actually sends, not prose. Non-OpenAI providers rarely share this shape; they fail closed (no silent wrong retry).

What we did not do (on purpose)

  • No separate restricted-models file (would fork source of truth from openai-models.yml)
  • No runtime “learn once, remember” write-back (your note on immutable config — happy to do a follow-up if product wants an in-memory discovery cache)
  • No multi-provider error taxonomy beyond OpenAI-shaped JSON + prose fallback

Happy to iterate if you and @alexheifetz want restrictions expressed as an explicit unsupportedParams: [...] list in YAML instead of the boolean supports_* flags — the wiring is already registry-driven.

Ready for re-review when convenient.

@igordayen

Copy link
Copy Markdown
Contributor

2. Fall back to the old Unsupported value: '…' regex for unstructured messages

@arimu1 - thank you for addressing the inquiries.
Do you still need unstructured message fallback?

@igordayen

igordayen commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Reasoning on Spring AI behavior for temperature support (generated by GPT):

The short answer is: it's not a Spring AI limitation. It is an OpenAI model capability limitation, and Spring AI 2.0 intentionally exposes only the parameters that the underlying model accepts.

The confusion comes from the fact that there are now two families of GPT models.

Model family | temperature | Why -- | -- | -- GPT-4o, GPT-4.1, GPT-5 Chat | ✅ Supported | Traditional autoregressive chat models GPT-5, GPT-5-mini, GPT-5-nano (reasoning) | ❌ Not supported | Reasoning models have fixed sampling behavior

Spring AI documents this explicitly:

When using GPT-5 models such as gpt-5, gpt-5-mini, and gpt-5-nano, the temperature parameter is not supported. Specifying it results in an API error. Conversely, gpt-5-chat supports temperature. (Home)

Why did OpenAI remove temperature?

This is an architectural decision.

Older GPT models generate tokens directly, so temperature modifies the probability distribution:

softmax(logits / temperature)

Higher temperature:

  • more randomness

  • more creative

  • less deterministic

Lower temperature:

  • more deterministic

  • better for code

  • less diverse

Reasoning models work differently.

Instead of directly sampling the answer, they internally perform reasoning/search/planning before producing the final response. OpenAI therefore controls the sampling strategy internally and exposes reasoning effort rather than sampling controls.

Instead of

temperature = 0.2

you tune

reasoning_effort = low | medium | high

which affects how much reasoning budget the model spends rather than how random token sampling is. (GitHub)

Why doesn't Spring AI silently ignore temperature?

Because that would hide configuration mistakes.

Suppose you configure

OpenAiChatOptions.builder()
    .model("gpt-5")
    .temperature(0.0)
    .build();

If Spring AI simply dropped the parameter, your application would behave differently from what you requested.

Instead it forwards the request to OpenAI, which returns

400
Unsupported parameter: 'temperature'

This makes the incompatibility obvious.

Why does Spring AI still have temperature() in OpenAiChatOptions?

Because one options class supports many models:

  • GPT-4o

  • GPT-4.1

  • GPT-5 Chat

  • Azure OpenAI

  • OpenAI-compatible providers

  • local OpenAI-compatible servers

Most of those do support temperature.

Spring AI cannot remove the field because it is valid for many models.

Is this ideal?

From an API-design perspective, probably not.

A stronger type-safe design would distinguish between model capabilities, for example:

interface SamplingCapability {
    Temperature temperature();
    TopP topP();
}

interface ReasoningCapability {
ReasoningEffort reasoningEffort();
}

or expose a per-model capability API such as:

chatModel.capabilities().supportsTemperature()
chatModel.capabilities().supportsReasoningEffort()

Then Spring AI (or your own abstraction) could validate options before sending the request.

In fact, this aligns well with the capability-based design you've been developing for caching (LLMCachingCapability) and other vendor-specific features. The same pattern could naturally extend to sampling, reasoning, structured output, streaming, and similar features.

The short answer is: it's not a Spring AI limitation. It is an OpenAI model capability limitation, and Spring AI 2.0 intentionally exposes only the parameters that the underlying model accepts.

The confusion comes from the fact that there are now two families of GPT models.

Model family temperature Why
GPT-4o, GPT-4.1, GPT-5 Chat ✅ Supported Traditional autoregressive chat models
GPT-5, GPT-5-mini, GPT-5-nano (reasoning) ❌ Not supported Reasoning models have fixed sampling behavior

Spring AI documents this explicitly:

When using GPT-5 models such as gpt-5, gpt-5-mini, and gpt-5-nano, the temperature parameter is not supported. Specifying it results in an API error. Conversely, gpt-5-chat supports temperature. ([Home]1)

Why did OpenAI remove temperature?

This is an architectural decision.

Older GPT models generate tokens directly, so temperature modifies the probability distribution:

softmax(logits / temperature)

Higher temperature:

  • more randomness
  • more creative
  • less deterministic

Lower temperature:

  • more deterministic
  • better for code
  • less diverse

Reasoning models work differently.

Instead of directly sampling the answer, they internally perform reasoning/search/planning before producing the final response. OpenAI therefore controls the sampling strategy internally and exposes reasoning effort rather than sampling controls.

Instead of

temperature = 0.2

you tune

reasoning_effort = low | medium | high

which affects how much reasoning budget the model spends rather than how random token sampling is. ([GitHub]2)

Why doesn't Spring AI silently ignore temperature?

Because that would hide configuration mistakes.

Suppose you configure

OpenAiChatOptions.builder()
    .model("gpt-5")
    .temperature(0.0)
    .build();

If Spring AI simply dropped the parameter, your application would behave differently from what you requested.

Instead it forwards the request to OpenAI, which returns

400
Unsupported parameter: 'temperature'

This makes the incompatibility obvious.

Why does Spring AI still have temperature() in OpenAiChatOptions?

Because one options class supports many models:

  • GPT-4o
  • GPT-4.1
  • GPT-5 Chat
  • Azure OpenAI
  • OpenAI-compatible providers
  • local OpenAI-compatible servers

Most of those do support temperature.

Spring AI cannot remove the field because it is valid for many models.

Is this ideal?

From an API-design perspective, probably not.

A stronger type-safe design would distinguish between model capabilities, for example:

interface SamplingCapability {
    Temperature temperature();
    TopP topP();
}

interface ReasoningCapability {
    ReasoningEffort reasoningEffort();
}

or expose a per-model capability API such as:

chatModel.capabilities().supportsTemperature()

chatModel.capabilities().supportsReasoningEffort()

Then Spring AI (or your own abstraction) could validate options before sending the request.

In fact, this aligns well with the capability-based design you've been developing for caching (LLMCachingCapability) and other vendor-specific features. The same pattern could naturally extend to sampling, reasoning, structured output, streaming, and similar features.


In the current agent-api, temperature is a generic property across all models / provides; see:
OptionsConverter.kt and LlmOptions.kt

Should it be this way, or to be removed from common interfaces and have a "minimum" interface to deal with this.
Is run-time detection the only option?
Obviously, compile-time control gets more complex because within OpenAI it behaves differently per model,

Could you please try to analyze the feasibility of compile-time control?

@alexheifetz - FYI

arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 3, 2026
Address @igordayen review on embabel#1852:

- Rename UnsupportedRequestParameterHandler → UnsupportedRequestParameterRetry
  (avoid "Handler" event/callback connotation)
- Expand KDoc: not a structured provider API; OpenAI/Azure message pattern;
  fail closed; strip when() maintenance notes; normalize is not Spring binder
- Clarify Prompt.options is required for strip retry (else rethrow)
- Rename SpecialHandlingConfiguration → SupportFeaturesConfiguration
  (YAML special_handling property name unchanged)

Tests: UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest,
CapabilityAwareOpenAiOptionsConverterTest, OpenAiModelLoaderTest green (JDK 21)

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 3, 2026
…of truth

Rebased on main (OptionsConverter 2-arg + model stamp). Prefer OpenAI JSON
error.param from Spring AI exception messages over English wording for the
one-shot retry safety net. Declarative omit via openai-models.yml
special_handling → ModelCapabilities remains the primary path (LLM model
database). Fail closed on non-retryable error codes.

Addresses igordayen design feedback on embabel#1852 / embabel#1724.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
arimu1 added a commit to arimu1/embabel-agent that referenced this pull request Aug 3, 2026
Prefer fail-closed extraction: only structured OpenAI JSON error.param
(with known codes) drives the one-shot strip/retry. Prose-only
"Unsupported value: '…'" messages no longer match. Model YAML
special_handling remains the primary defence (embabel#1724 / embabel#1852).

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1
arimu1 force-pushed the fix/1724-unsupported-request-params branch from 019aad8 to f631de1 Compare August 3, 2026 23:36
@arimu1

arimu1 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Good question — no, we no longer need the unstructured message fallback.

Change (tip after rebase)

Removed the Unsupported value: '…' regex path entirely. Extraction is now fail closed:

  1. Primary: model YAML special_handlingModelCapabilities → omit params before the call
  2. Safety net: parse Spring AI "NNN - {json}" for structured OpenAI error.param (+ known retryable error.code / missing code with present param)
  3. Otherwise: return null → rethrow (no prose matching)

Prose-only messages without JSON error.param no longer retry. Unit tests updated accordingly (UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest including a negative case for unstructured prose). 25 focused tests green (JDK 21).

Ready for re-review when convenient.

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arimu1 getting better. main inquiry on usability, to alert developer on earlier stages bu checking capabilities, getting error code or exception. thank you

Comment thread embabel-agent-openai/src/main/kotlin/com/embabel/agent/openai/converters.kt Outdated

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arimu1 - getting better, main inquiry is usability - so user can get early alerts - by checking capabilities, error code or exception, thank you

@arimu1

arimu1 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Thanks — pushed the early usability path:

Early prevention

  • New public UnsupportedParameterException(parameter, model) thrown from CapabilityAwareOpenAiOptionsConverter when a restricted non-default temperature or non-null topP / penalties would be sent.
  • Default temperature 1.0 (and nulls) still omitted without error (providers often only reject non-default temperature).
  • ModelCapabilities stays the public probe surface for proactive checks before building options.

Cleanup (review threads)

  • Dropped issue-number references in code comments.
  • Clarified strip scope (portable ChatOptions sampling mutators only) and why top_p vs topp both appear after normalize.
  • Documented that stripParameter returns null only for unknown field names (cannot invent options); already-null fields still build a copy.
  • Kept fail-closed tests for prose-only errors intentionally (not leftover unstructured matching).
  • ChatOptions.model remains nullable in the ToolCalling fallback path.

Tests

CapabilityAwareOpenAiOptionsConverterTest + Gpt5ChatOptionsConverterTest + UnsupportedRequestParameterRetryTest + InstrumentedChatModelTest green (JDK 21).

Tip: 97bc6e856. Happy to iterate if you want fail-on-default-temperature too, or a richer capabilities API (e.g. streaming flags) as a follow-up.

@arimu1

arimu1 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Addressed remaining review threads on the retry safety-net:

  1. Comments / scope — KDoc now states this is only a portable ChatOptions mutator safety-net (not a product “hyperparams” abstraction); primary path remains ModelCapabilities + UnsupportedParameterException before the call.
  2. a_b normalizenormalize now strips _/-/spaces so top_p / topP both become topp (single when arm).
  3. Omitted param / throwstripParameter returning null still means unknown field name → rethrow original (we never invent options). Already-null known fields still build a copy for a single harmless retry.
  4. model? nullable — Spring AI ChatOptions.model is String?; we only copy when non-null (comment clarified).
  5. Prose-only tests — kept as fail-closed contract tests (explicitly not leftover unstructured matching).

Early UnsupportedParameterException via capabilities remains the preferred path from the previous tip.

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arimu1 - thanks for the next iteration. this PR is tagged for this week release. Please try to address inquiries. and plan - as research for now - capabilities check, see
#1585 Design: Capability-based model abstraction

also - could you mark items as "resolved" - whatever gets actually resolved.
Regards.

Comment thread embabel-agent-openai/src/main/kotlin/com/embabel/agent/openai/converters.kt Outdated
Comment thread embabel-agent-openai/src/main/kotlin/com/embabel/agent/openai/converters.kt Outdated
@igordayen

Copy link
Copy Markdown
Contributor

@arimu1 - please get familiar with #1874.
would be good to be consistent.
using kotlin require is actually good practice.
Could you please expedite this PR, as it gets targeted for this week release, Thanks

arimu1 added 4 commits August 6, 2026 08:01
Generalize temperature-only converter split into capability-aware
OpenAI options conversion, mark GPT-4.1 family as temperature-restricted,
avoid re-adding omitted params when attaching tools, and retry once in
InstrumentedChatModel when the provider returns Unsupported value: 'param'.

Fixes embabel#1724

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
Address @igordayen review on embabel#1852:

- Rename UnsupportedRequestParameterHandler → UnsupportedRequestParameterRetry
  (avoid "Handler" event/callback connotation)
- Expand KDoc: not a structured provider API; OpenAI/Azure message pattern;
  fail closed; strip when() maintenance notes; normalize is not Spring binder
- Clarify Prompt.options is required for strip retry (else rethrow)
- Rename SpecialHandlingConfiguration → SupportFeaturesConfiguration
  (YAML special_handling property name unchanged)

Tests: UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest,
CapabilityAwareOpenAiOptionsConverterTest, OpenAiModelLoaderTest green (JDK 21)

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
…of truth

Rebased on main (OptionsConverter 2-arg + model stamp). Prefer OpenAI JSON
error.param from Spring AI exception messages over English wording for the
one-shot retry safety net. Declarative omit via openai-models.yml
special_handling → ModelCapabilities remains the primary path (LLM model
database). Fail closed on non-retryable error codes.

Addresses igordayen design feedback on embabel#1852 / embabel#1724.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
Prefer fail-closed extraction: only structured OpenAI JSON error.param
(with known codes) drives the one-shot strip/retry. Prose-only
"Unsupported value: '…'" messages no longer match. Model YAML
special_handling remains the primary defence (embabel#1724 / embabel#1852).

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
arimu1 added 3 commits August 6, 2026 08:01
Address usability feedback: fail at options conversion when a restricted
non-default sampling value is set, instead of silent omit + log only.
ModelCapabilities remains the public probe surface; HTTP strip/retry stays
a fail-closed safety net for structured error.param only.

- Add UnsupportedParameterException (parameter + model)
- CapabilityAwareOpenAiOptionsConverter throws on unsupported non-default
  temperature / non-null topP and penalties
- Clarify strip/normalize docs; drop issue-number code comments
- Update Gpt5 / capability converter tests

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
Address review feedback on UnsupportedRequestParameterRetry:
- Document scope as portable ChatOptions mutators (not product hyperparams)
- Normalize param names by stripping all separators (top_p/topP -> topp)
- Clarify fail-closed prose tests and nullable ChatOptions.model copy

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
… capabilities

Be consistent with the Responses/GPT-5 work (embabel#1874): map the token limit to
max_completion_tokens for the GPT-5 family, expand per-model special_handling
when a tier refuses all sampling fields, and keep require-style early fails via
UnsupportedParameterException (typed IllegalArgumentException).

Also document the option-conversion → HTTP safety-net chain, share sampling
param name constants, clarify stripParameter null semantics for the retry path,
and note the relationship to broader capability design (issue 1585).

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1
arimu1 force-pushed the fix/1724-unsupported-request-params branch from 2ec39f7 to 5246b91 Compare August 6, 2026 01:04
@arimu1

arimu1 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Expedited for this week's release — tip 5246b91a6 (rebased on current main).

Consistency with #1874

  • GPT-5 family models now map the token limit to max_completion_tokens (uses_max_completion_tokens: true in YAML / ModelCapabilities.usesMaxCompletionTokens) and leave max_tokens unset — same refusal we would otherwise hit live.
  • Per-model special_handling expanded for tiers that refuse all sampling fields (gpt-5 / mini / nano / pro / 5.3-chat); 5.1 / 5.2 / 5.4 stay temperature-restricted only (they still accept top_p / penalties per the live checks on Fix/1758 OpenAI pro responses api #1874).
  • Gpt5ChatOptionsConverter alias uses ModelCapabilities.GPT5_FAMILY (no sampling + max completion tokens). Registration still goes through per-model YAML → CapabilityAwareOpenAiOptionsConverter.

Require-style fail-fast (vs warn-and-ignore)

  • Kept early fail: non-default unsupported sampling values throw UnsupportedParameterException (now extends IllegalArgumentException — same family as Kotlin require).
  • Structural preconditions use require(...) (non-blank model id / parameter name).
  • Default temperature 1.0 / nulls still omitted silently so default configs keep working.
  • This is the intentional contrast with Fix/1758 OpenAI pro responses api #1874's warn-and-drop for temperature: developers get the earliest alert at option conversion. Happy to align Fix/1758 OpenAI pro responses api #1874 the same way if you want one behavior for the release.

Review threads addressed

  • Shared SamplingParameterNames constants.
  • Documented the full chain: LlmOptions → converter (throw here) → HTTP → InstrumentedChatModel structured error.param safety-net only.
  • Clarified stripParameter null = unknown field name (rethrow); known-but-already-null still returns a copy for one harmless retry — not “field was omitted so throw”.
  • Retry stays a thin safety net; prefer extending YAML capabilities over growing the strip map.
  • Prose-only tests kept as fail-closed contract tests (not leftover unstructured matching).
  • ChatOptions.model remains nullable; only copied when non-null in the tool-options fallback.

#1585 (capabilities design) — research note, not a rewrite

ModelCapabilities here is intentionally the narrow sampling/token slice so this fix can ship for the release. The composable trait model in #1585 (TextCompletion / StreamingTextCompletion / GenericModel.get<T>()) is the longer-term home for “does this model support X?” (streaming, modalities, etc.). Natural evolution later: project these YAML flags (and streaming/thinking) through that trait surface so callers can probe before building options — without waiting on the full rewrite for this week's fix.

Tests (JDK 21)

CapabilityAwareOpenAiOptionsConverterTest, Gpt5ChatOptionsConverterTest, StandardOpenAiOptionsConverterTest, UnsupportedRequestParameterRetryTest, InstrumentedChatModelTest, SpringAiLlmMessageSenderTest, OpenAiModelLoaderTest — green.

All open review threads on this PR marked resolved for the fixed items. Ready for re-review / merge when convenient.

@igordayen

igordayen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • LlmOptions → converter (throw here) → HTTP → InstrumentedChatModel structured error.param safety-net only.

@arimu1

This JUNIT to produce an exception is a good one:

         val options = LlmOptions()
                .withTemperature(0.5)
                .withTopP(0.8)
                .withMaxTokens(200)
                .withFrequencyPenalty(0.4)
                .withPresencePenalty(0.3)

            val ex = assertThrows(UnsupportedParameterException::class.java) {
                converter.convertOptions(options, "restricted-model")
            }

But I actually was looking for a test that starts with

prompRunner.withLlm(...).createObject(prompt, SomeClass.class)

to ensure the exception will be propagated and caught there.

From your comments, I'm not sure I fully understand whether you are considering using Kotlin "require" or not, and what the remaining items (if any) are.
Thank you!

@igordayen

Copy link
Copy Markdown
Contributor

@arimu1 - could you please respond, thank you

@arimu1

arimu1 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

@igordayen Sorry for the delay — answering your questions:

Kotlin require vs remaining items

Yes — this branch uses require-style fail-fast at option conversion:

  • UnsupportedParameterException (extends IllegalArgumentException) for sampling params the model rejects
  • require(model.isNotBlank()) on the converter entry
  • Converter throws before HTTP; InstrumentedChatModel structured error.param path remains a safety-net only (not the primary control)

Chain (documented on the converter):
LlmOptionsCapabilityAwareOpenAiOptionsConverter (throw here) → HTTP → InstrumentedChatModel safety-net.

Unit coverage for the throw is on the converter (assertThrows(UnsupportedParameterException…) with the multi-param LlmOptions setup you quoted).

promptRunner.withLlm(...).createObject IT

Understood — you want end-to-end propagation through the AI façade, not only the converter unit test. I will add that IT next (assert UnsupportedParameterException bubbles from promptRunner.withLlm(...).createObject(...) for a restricted model).

Rebase note after #1874

PR is currently CONFLICTING with main after #1874 landed (Gpt5ChatOptionsConverter on main now warns and drops sampling params rather than throwing).

Before I force-resolve the rebase: do you still want #1852’s throw/UnsupportedParameterException behavior for capability-flagged models, or should this PR be narrowed / closed as superseded by the #1874 ignore-with-warn approach for the GPT-5 path?

Happy to implement either once you confirm. Thanks.

@igordayen

igordayen commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Rebase note after #1874

PR is currently CONFLICTING with main after #1874 landed (Gpt5ChatOptionsConverter on main now warns and drops sampling params rather than throwing).

Before I force-resolve the rebase: do you still want #1852’s throw/UnsupportedParameterException behavior for capability-flagged models, or should this PR be narrowed/closed as superseded by the #1874 ignore-with-warn approach for the GPT-5 path?

Happy to implement either once you confirm. Thanks.

Looping @alexheifetz

@arimu1 - @azanux presented a very strong argument against the exception (cost-related) and can't beat it:)

Let's plan a strategically proper solution using the capabilities API

Please go with the originally planned warning and rebase.

Appreciate accommodating requests "as we go along the journey" :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generically handle models that reject unsupported request parameters (e.g. temperature)

2 participants