Repository navigation
feat(session): fill omitted sampling fields from defaults set at session creation - #3585
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review always |
e65c7ce to
d59aaec
Compare
…ion creation Agent frameworks may drop the sampling values Miles hands them in request_kwargs, so a chat request can reach the engine without temperature/top_p/top_k and sample at the model's generation_config or SGLang's defaults while training scales logits by --rollout-temperature. An engine-level default cannot express this either: rollout and eval share engines and eval datasets differ per sample, so the value is a property of the session, not the engine. POST /sessions now accepts temperature, top_p and top_k next to evaluation. The agentic generator passes the sample's resolved sampling_params (rollout flags for training, the dataset's values for eval), the session keeps them for its lifetime, and prepare_chat_request fills the fields a request leaves unset, after the server rules and before the model rules so turn_args records the effective request. An explicit request value still wins; null counts as unset, matching the engine's own chat semantics. request_kwargs to the agent and the built-in /generate path are unchanged, and nothing is added to engine launch arguments. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CreateSessionRequest.sampling_defaults() only re-expressed pydantic's model_dump(include=..., exclude_none=True). The route now calls that directly and the request model keeps only its fields. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The tracer validates sampling_params through CreateSessionRequest and sends its dump; the route dumps the same model minus evaluation. The field list therefore lives only on the model, and SESSION_SAMPLING_FIELDS goes away. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The eval table documents --eval-temperature and --eval-top-p but not --eval-top-k, although the parser registers all three with the same rollout fallback. A reader checking which eval sampling flags exist could not find top-k. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An eval dataset YAML can spell top_k as 40.0. EvalDatasetConfig keeps the float, and since the tracer now validates sampling_params through CreateSessionRequest, StrictInt rejected it: every group of that dataset failed at session creation and was dropped with one error log, where the same config generated before this branch. The request model now folds an integral float into an int before validation; 20.5, booleans and strings are still rejected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
fbd6d8b to
8c897ff
Compare
There was a problem hiding this comment.
The prior finding is fixed: OpenAIEndpointTracer.create now builds its POST /sessions body via CreateSessionRequest.model_validate(...) (openai_endpoint_utils.py:60-61), so an eval dataset's top_k: 40.0 is coerced to int by the same validator used at session creation, with a new regression test (test_an_integral_float_top_k_is_stored_as_an_int). I didn't find anything else to raise.
Extended reasoning...
Since my prior review, the author pushed 5 commits, the last of which (8c897ff) fixes exactly the float-top_k ValidationError I flagged: openai_endpoint_utils.py now routes the outgoing session-creation body through CreateSessionRequest.model_validate, reusing types.py's integral-float coercion, and a new test in test_session_evaluation.py covers it directly. The other new commits are small refactors (deriving the creation body from the request model, inlining sampling fields, a docs update) that don't introduce new surface area. This is the third+ pushed version I've reviewed; given the fix is concrete and tested and no new concerns surfaced, this note is informational rather than a fresh objection, but a human should still confirm the fix before merge given the session/sampling code is security/behavior sensitive for training correctness.
| A field the client sent with any value but ``None`` is kept; ``None`` counts as unset, | ||
| as it does for the engine's own chat defaults. | ||
| """ | ||
| for key, value in sampling_defaults.items(): |
There was a problem hiding this comment.
qq: curious why don't we add an assertion/warning between the saved value and the request_args[key], if it is not none? Is the agent changing the sampling params expected?
There was a problem hiding this comment.
codex cmt
Added a hard gate in b65b0b4ad: training requests return HTTP 400 when an explicit temperature differs from the registered value; omitted/null values use the registered value. Evaluation can still override temperature.
top_p/top_k remain overridable because the realized sampling mask is the source of truth for replayed support.
Verified v1/v2 continuations, retries, and recovery after rejection: 168 passed, 7 skipped in the targeted CPU/mock-backend tests.
8c897ff to
3f565e1
Compare
Keep creation defaults, explicit request precedence, and evaluation capture limits concise in the agentic rollout guide.
Reject explicit training temperature mismatches with HTTP 400 before forwarding or mutating session state. Preserve omitted/null defaults, evaluation overrides, and independent top-p/top-k settings. Filter engine-only sampling fields before strict session-creation validation. Cover v1/v2 first turns, continuations, retries, and recovery after rejection.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
I reviewed this PR again on the latest push. Beyond the docs inline nit below, a human should specifically re-check one item that this round's pipeline marked as "ruled out": the concern that OpenAIEndpointTracer.create (miles/rollout/generate_utils/openai_endpoint_utils.py:61) spreads the caller's entire sampling_params dict into CreateSessionRequest.model_validate. Tracing the actual call path (compute_sampling_params / GenerateState.sampling_params -> agentic_tool_call.generate -> OpenAIEndpointTracer.create), that dict always carries non-sampling keys such as max_new_tokens, stop, stop_token_ids, skip_special_tokens, etc. CreateSessionRequest is a StrictBaseModel (extra="forbid") that only declares evaluation/temperature/top_p/top_k, and I don't see any filtering step added between the sampling_params dict and model_validate. From the code alone this still looks like it would raise a pydantic ValidationError on essentially every agentic-tool-call session creation, not just the misconfigured-eval-dataset case the earlier objection was framed around. Worth confirming with an actual run of tests/fast/rollout/generate_utils/test_openai_endpoint_utils.py before merging.
Extended reasoning...
The diff plumbs session-level sampling defaults (temperature/top_p/top_k) through CreateSessionRequest, OpenAIEndpointTracer.create, the /sessions route, LinearTrajectory/SessionStateV2, and prepare_chat_request, with matching tests and docs; it touches no auth/crypto surface but does sit on the core rollout request path used by every agentic sample. An earlier round's objection that OpenAIEndpointTracer.create spreads the full sampling_params dict into the strict, extra-forbidding CreateSessionRequest was marked resolved by the author and then classified as ruled-out this run, but my own trace of compute_sampling_params -> GenerateState.sampling_params -> agentic_tool_call.generate -> OpenAIEndpointTracer.create shows the non-sampling keys (max_new_tokens, stop, etc.) are still spread unfiltered into model_validate, with no filtering step added anywhere in the diff. I was not able to execute the test suite in this environment to confirm the ValidationError actually fires, which is why I'm flagging it for a human to verify rather than asserting it outright.
| Use `{"evaluation": true}` in `POST /sessions` for evaluation; omitting it defaults to training. The agentic generator sets this automatically, and the purpose stays fixed across turns, retries, and branches. | ||
|
|
||
| Evaluation sessions force `return_sampling_mask`, `return_routed_experts`, and `return_indexer_topk` to `false`, including when a request or model rule asks to enable them. They ignore `routed_experts_start_len`. Sampling defaults and override order stay unchanged, and temperature can vary between turns. TITO rendering, token IDs, logprobs, and template compatibility checks still apply; agentic evaluation still collects samples. | ||
| `temperature`, `top_p`, and `top_k` in `POST /sessions` provide defaults for omitted or `null` chat fields; explicit request values win. The agentic generator registers the sample's resolved values automatically. |
There was a problem hiding this comment.
🟡 nit (optional): New line 138 says session-level temperature/top_p/top_k now fill omitted chat fields, but line 131 in the same doc ('Choose template options per session') still states 'Sampling parameters remain per-request', unchanged by this PR. A reader hits contradictory claims about the same behavior a few lines apart. Fix: update line 131 to note that sampling parameters are per-request except for fields a session registers as defaults, so the two sections agree.
Why this was flagged
docs/user-guide/agentic-rollout.md:131 (unchanged by this diff) asserts 'Sampling parameters remain per-request' under the 'Choose template options per session' heading. This PR adds docs/user-guide/agentic-rollout.md:138 stating temperature/top_p/top_k in POST /sessions now provide session-level defaults for omitted chat fields, which is exactly a per-session (not per-request) behavior. A maintainer or integrator reading the file top to bottom gets two directly conflicting statements about the same feature within one page, with no cross-reference resolving them. Nothing in the diff updates line 131 to reflect the new session-level default path introduced alongside line 138.
Verification: nit. docs/user-guide/agentic-rollout.md:131 (unchanged by this PR) states "Sampling parameters remain per-request." while the PR adds line 138: "temperature, top_p, and top_k in POST /sessions provide defaults for omitted or null chat fields; explicit request values win." Both statements are present and describe the same fields (temperature/top_p/top_k). Line 138 introduces… | nit.…
2 participants
Summary
Register session sampling defaults and enforce a fixed training temperature.
Motivation
Agent frameworks can drop the sampling values Miles supplies in
request_kwargs. A request then reaches the engine withouttemperature,top_p, ortop_k, while training still scales logits by--rollout-temperature, making the rollout silently off-policy. The value belongs to the session because rollout and dataset-driven eval may share an engine but require different sampling settings.Usage
Omitted or
nullfields use these defaults. A training request withtemperature: 0.1returns HTTP 400; evaluation temperature andtop_p/top_kremain overridable.Design Notes
GenerateFnInput.sampling_paramspasses throughOpenAIEndpointTracer.createinto validated v1/v2 session state; each chat fills omitted fields and checks training temperature before model rules, rendering, or forwarding, without changing engine launch arguments.top_p/top_kcan vary because replay uses the realized sampling mask.max_new_tokensbefore validation.preferred_sampling_paramsmerge provides fallback semantics, but session-owned defaults support different rollout/eval settings on a shared engine.Verification
/opt/sglang/bin/python -m pytest tests/fast/router/test_session_evaluation.py tests/fast/rollout/session/test_request_args.py tests/fast/rollout/generate_utils/test_openai_endpoint_utils.py tests/fast/rollout/generate_hub/test_agentic_v2.py tests/fast/rollout/generate_hub/test_multi_turn.py -q --tb=short --durations=5: 168 passed, 7 skipped againstb65b0b4adfd60cef18bfad99d4e29f393137c4c7, covering v1/v2 gates, retries, defaults, tracer filtering, and agentic multi-turn requests.ruff,autoflake,isort,black, andgit diff --checkpassed on the gate changes.Review Focus
apply_session_sampling_defaults: reject temperature mismatches before backend forwarding or session-state mutation.OpenAIEndpointTracer.create/CreateSessionRequest: filter engine-only fields while retaining strict public creation validation.