Skip to content

test(session): keep training-session sampling tests unbounded - #3595

Closed
guapisolo wants to merge 1 commit into
mainfrom
jiajun/session-eval-unbounded-sampling
Closed

guapisolo wants to merge 1 commit into
mainfrom
jiajun/session-eval-unbounded-sampling

Conversation

@guapisolo

Copy link
Copy Markdown
Collaborator

Summary

Training-session tests use unbounded sampling; bounded values stay in evaluation sessions.

Motivation

Sampling-support replay (#3354 and its follow-up nanjiangwill#2) adds a session-server rule: on a run whose launch flags do not capture rollout sampling support (--rollout-top-p 1.0, --rollout-top-k -1), a training request that samples from a bounded support (top_p < 1 or top_k > 0) is rejected with HTTP 400 bounded training-request sampling requires bounded rollout sampling, because the trainer would score the full vocabulary while the engine truncated the distribution. Evaluation sessions are exempt. Several tests on main create training sessions with incidentally bounded values; rebased onto that stack, six cases of test_training_temperature_mismatch_is_rejected_before_forwarding fail with that error although boundedness is not what they test. Public API, production code, and observable server behavior are unchanged; only test data moves.

Before / After

  • Before / After: One bounded SAMPLING constant served both training and evaluation sessions; now SAMPLING = {"temperature": 0.6, "top_p": 1.0, "top_k": -1} serves training sessions while the new EVAL_SAMPLING = {"temperature": 0.6, "top_p": 0.9, "top_k": 20} serves the evaluation branch of the shared default-filling test.
  • What moved where: Test data only, in tests/fast/router/test_session_evaluation.py (the constants above; the temperature-mismatch follow-up chat top_p=0.5 -> top_p=1.0; the top_k: 40.0 coercion test created as an evaluation session, matching the eval dataset YAML in its docstring) and tests/fast/rollout/session/test_request_args.py (the prepare_chat_request default-filling test uses unbounded sampling_defaults for training, ahead of the planned move of validation after default filling); no production module changes.

Behavior Preservation

  • test_creation_sampling_defaults_fill_only_omitted_fields[False|True]: omitted fields are filled from creation-time defaults; the evaluation branch still overrides temperature and top_k, the training branch restates its session values (a training request cannot legally differ from them).
  • test_training_temperature_mismatch_is_rejected_before_forwarding[*]: a mismatched temperature is still rejected before forwarding, and the matching request still reaches the wire as (saved, 1.0, -1).
  • test_training_null_temperature_uses_registered_value, test_creation_without_sampling_defaults_leaves_omitted_fields_unset: assertions unchanged against the unbounded SAMPLING.
  • test_an_integral_float_top_k_is_stored_as_an_int: 40.0 still reaches the engine as int 40, now from an evaluation session.
  • test_request_args.py::test_session_sampling_defaults_fill_only_omitted_fields[False|True]: explicit temperature and top_k kept, omitted top_p filled from policy-matching defaults.

Verification

Devbox miles-nda-h200, /opt/sglang/bin/python (torch 2.13.0+cu130), python -m pytest -q tests/fast/router/test_session_evaluation.py tests/fast/rollout/session:

  • main (366f614) + this commit: 231 passed, 0 failed (test_session_evaluation.py 65, test_request_args.py 39, tests/fast/rollout/session 166).
  • [Sampling Replay] Bounded sampling-support replay #3354 stack rebased on main (local tip d7658c131e) without this commit: 6 failed, 239 passed; the six are test_training_temperature_mismatch_is_rejected_before_forwarding[v1|v2-*].
  • Same rebased stack with this commit cherry-picked: 245 passed, 0 failed (test_session_evaluation.py 65, test_request_args.py 41, tests/fast/rollout/session 180).
  • black==24.3.0 --check, isort==5.13.2 --check-only, ruff==0.14.7 check on both files: clean.

Review Focus

  • SAMPLING / EVAL_SAMPLING in tests/fast/router/test_session_evaluation.py: every session created without "evaluation": true must stay unbounded; bounded values belong only to evaluation sessions.
  • test_creation_sampling_defaults_fill_only_omitted_fields[False]: the training branch's explicit chat now restates the defaults, so the discriminating override lives in the evaluation branch by design.

🤖 Generated with Claude Code

Sampling-support replay (PR #3354 and its follow-up) makes the session
server reject a training request that samples from a bounded support
(top_p < 1 or top_k > 0) on a run whose launch flags do not capture
rollout sampling support, because the trainer would score the full
vocabulary while the engine truncated the distribution. Several session
tests create training sessions with incidentally bounded values and
would start failing once that rule lands, even though boundedness is not
what they test.

Move the training-session test data to unbounded values (top_p 1.0,
top_k -1) while keeping each test's purpose: default filling of omitted
fields, temperature-mismatch rejection before forwarding, the null
temperature fallback, and the integral-float top_k coercion (now created
as an evaluation session, matching the eval dataset YAML scenario in its
docstring). Evaluation sessions keep bounded values through a separate
EVAL_SAMPLING constant. The prepare_chat_request unit test gets the same
split so bounded defaults never reach a training session there either.

No production code changes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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