Skip to content

feat: pass temperature, top_p, stop and seed to the model - #141

Open
coyl wants to merge 3 commits into
mainfrom
feat/sampling-params
Open

coyl wants to merge 3 commits into
mainfrom
feat/sampling-params

Conversation

@coyl

@coyl coyl commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

temperature, top_p, stop and seed reach the model that serves the call. Until now /v1/chat/completions and /v1/messages accepted them and dropped them.

What changes

  • API. GenerateTextRequest gets temperature = 18, top_p = 19, stop = 20 and seed = 21. Field 17 is left for feat(routing): data_classification restricted keeps calls on self-hosted nodes #139.
    • Chat completions takes all four; stop can be a string or an array.
    • /v1/messages takes temperature, top_p and stop_sequences; top_k is still ignored.
    • Bounds: temperature 0..2, top_p 0..1, up to 16 non-empty stop sequences. Anything else gets INVALID_ARGUMENT; batch items are checked the same way.
    • Over HTTP, empty stop sequences are dropped instead of rejected, because clients sent them while the field was ignored.
  • One table decides what each model gets. registry.SamplingSupport(m) gives each adapter its filter at build time.
    • The OpenAI-compatible adapter covers nodes too, through ConfigureOpenAI.
    • The handler computes the ignored list from the same table for the model that answered, so a fallback reports its own.
    • The list goes out as ignored_parameters: in the response and on the first stream chunk, cached replays included. Over HTTP it is the X-KTMS-Ignored-Params header.
  • Per vendor (see README "Sampling"):
    • OpenAI and Azure: all four, up to 4 stop sequences. Reasoning models (o<digit> or gpt-5 at the start of the id or after /, - or _, except -chat) get none, since they reject them.
    • Groq: all four, up to 4 stop sequences.
    • Google: all four, up to 5 stop sequences, including the JSONL batch path.
    • Ollama and local nodes: all four.
    • DeepSeek: no seed.
    • Mistral: no seed, because its API calls it random_seed; temperature is capped at 1.5.
    • Claude (Anthropic and Bedrock): stop_sequences always, minus whitespace-only sequences, which Claude rejects. Temperature (capped at 1) and top_p only up to 4.6: the SDK sends API version 2023-06-01, and 4.7 and later accept only default sampling. From 4 on the two together are rejected, so top_p is dropped when both are set; Claude 3 keeps both.
    • Other Bedrock models: temperature (capped at 1), top_p, up to 4 stop sequences.
  • When the id misleads. Two new capabilities correct the table:
    • no_sampling, for a model whose API rejects what the table would send;
    • sampling, for a model that takes its vendor's fields although its id looks otherwise: an Azure deployment name, a Claude alias, a gpt-5 run without reasoning.
  • Cache key. Sampling joins the exact and semantic keys. A field is written to the key only when set, so existing keys stay valid.

Behaviour change

A client that already sends temperature (many SDKs do by default) now gets it applied. This is in CHANGELOG under Changed. The plugin skills assess and migrate now say 1.4 and earlier ignore these fields.

Not in this PR

  • Per-rule or per-model defaults and caps for sampling. The issue proposed them; they can follow if wanted.
  • Per-item ignored_parameters in batch results.
  • /v1/responses sampling, which arrives with that endpoint.

Review

Ten findings from /code-review.

Fixed:

  • dotted Claude ids (claude-opus-4.7) were treated as 4.6 or earlier;
  • prefixed OpenAI and Azure reasoning ids were missed;
  • whitespace-only stop sequences failed on Claude;
  • top_p was dropped on Claude 3;
  • there was no way to turn sampling on for a model whose id misleads;
  • HTTP rejected empty stop sequences.

Also documented: batch results carry no ignored_parameters, and the 400s for out-of-range values over HTTP.

Left as is:

  • The ignored list is computed again from the same SamplingSupport(m) the adapter was built with. It is one function with one model, so the two cannot drift, and the cost is a regex per response.
  • The suggested shared parser for "string or array". Folding schemaType into it would change what a "null" type returns.

Checks

go vet ./..., go test ./... -race, and TestSkill*.

Tests cover:

  • Apply;
  • the vendor table;
  • what each adapter puts on the wire (OpenAI, Anthropic, Gemini, Bedrock);
  • gRPC end to end against a fake OpenAI vendor (a chat model and a reasoning model, stream first chunk, validation);
  • HTTP chat and Messages;
  • the cache key.

Closes ai-process/docs#186

Review, second pass (with #139, docs#202 and docs#193)

Open, by severity:

  1. Conflicts with feat(routing): data_classification restricted keeps calls on self-hosted nodes #139. Both PRs add fields to GenerateTextRequest (17 there, 18–21 here) and touch gateway.pb.go, grpcapi/batch.go and server/openai_test.go; git merge-tree reports conflicts in those four files. Whichever lands second must be rebased and make proto re-run: a hand-merged raw descriptor in gateway.pb.go can panic at init or drift from the .proto.
  2. A capped temperature is silent (internal/llm/sampling.go, Apply). Validation accepts 0..2 for every model, but a value above a vendor's cap (Claude, Bedrock, Mistral) is lowered without a trace: temperature=1.8 on claude-sonnet-4-6 runs at 1.0 with empty ignored_parameters and no X-KTMS-Ignored-Params. The one feedback channel for sampling says nothing about a changed value. Options: report temperature in the list when it was capped, or a separate adjusted_parameters.
  3. OneOfTemperatureTopP on every Claude 4.x (internal/registry/sampling.go, claudeSampling). The reviewer believes the "not both" rule started with Opus 4.1, so claude-sonnet-4-20250514 / claude-opus-4-20250514 lose top_p for no reason. To confirm against the Anthropic docs before changing.
  4. ignoredParameters recomputes the list (internal/grpcapi/sampling.go). The adapter already ran a.sampling.Apply(...) and threw the list away; the handler runs registry.SamplingSupport(m).Apply again. Two computations of one fact, plus regex matching on the hot path. Returning the list from the adapter (on llm.Response / the first chunk) would leave one. Kept as is for now: same function, same model, so they cannot drift today.
  5. top_k on /v1/messages is dropped silently (internal/server/anthropic.go). The header is documented as the list of sampling fields the model did not get, yet top_k is accepted, never sent, and not reported, unlike temperature, top_p and stop_sequences in the same request.
  6. Comment length. AGENTS.md §6 allows 3 lines; the comment on SamplingSupport.Apply (internal/llm/sampling.go) is 4.

Resolved in 5b0e3b2:

  1. Conflicts with feat(routing): data_classification restricted keeps calls on self-hosted nodes #139: still open. Whichever PR lands second gets rebased and buf generate re-run, with no hand-merged gateway.pb.go.
  2. A capped temperature is now reported as temperature in ignored_parameters and the header. The proto, README and docs say so.
  3. Checked against Anthropic's migration guide, no change: passing both temperature and top_p errors on every Claude 4+ model, including Sonnet 4 and Opus 4. Claude 3 keeps both.
  4. The handler now skips the vendor table when the call sets no sampling, so there is no regex work on the common path. Returning the list from the adapter stays a possible follow-up: same function, same model, no drift today.
  5. top_k on /v1/messages is named in X-KTMS-Ignored-Params, in plain and streamed responses.
  6. The comment on Apply is down to 3 lines.

Checked and fine: every vendor of the OpenAI adapter is covered by SamplingSupport, the semantic cache includes sampling in its key through writeParams, and X-KTMS-Ignored-Params is set before the first SSE chunk.

GenerateTextRequest, chat completions and /v1/messages carry sampling to
the serving model as far as its API takes it. A per-vendor table
(registry.SamplingSupport) decides what each model gets; the adapter
leaves out the rest and the response names it in ignored_parameters and
X-KTMS-Ignored-Params. The no_sampling capability sends a model none.
Sampling joins the response cache key.
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 29, 2026, 1:30 PM

…bility

Dotted Claude ids no longer pass for 4.6 and earlier, prefixed OpenAI
reasoning ids are recognised, and Claude 3 keeps top_p beside
temperature. Claude gets no whitespace-only stop sequence. The sampling
capability turns the vendor's fields on for a model whose id misleads.
HTTP drops empty stop sequences, as clients sent them while ignored.
A temperature lowered to the vendor's cap is named in
ignored_parameters, and /v1/messages names top_k, which no model gets.
The handler skips the vendor table when the call sets no sampling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant