feat(sglang): plumb per-request sampling_seed for replayable sampling - #1524
feat(sglang): plumb per-request sampling_seed for replayable sampling#1524SushantDaga wants to merge 7 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a per-request sampling_seed for replayable sampling on the SGLang backend, along with an enable_deterministic_inference flag in SGLangConfig to ensure SGLang honors the seed. If sampling_seed is set but deterministic inference is disabled, a warning is raised, while the vLLM backend raises a NotImplementedError if a seed is provided. The review feedback points out that the warning in __post_init__ should also check eval_gconfig.sampling_seed in addition to gconfig.sampling_seed, and suggests adding a corresponding test case to verify this behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if ( | ||
| self.gconfig.sampling_seed is not None | ||
| and not self.sglang.enable_deterministic_inference | ||
| ): | ||
| warnings.warn( | ||
| "gconfig.sampling_seed is set but sglang.enable_deterministic_inference " | ||
| "is False: SGLang silently ignores per-request sampling_seed unless the " | ||
| "server is launched with --enable-deterministic-inference. Rollouts will " | ||
| "not be seeded as expected. (Not applicable if you're launching SGLang " | ||
| "servers yourself outside this config, or using the vLLM backend, which " | ||
| "rejects sampling_seed outright.)", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
The warning only checks self.gconfig.sampling_seed, but self.eval_gconfig can also have sampling_seed set. If self.eval_gconfig.sampling_seed is set without sglang.enable_deterministic_inference being enabled, the evaluation rollouts will silently run without deterministic sampling. We should check both self.gconfig.sampling_seed and self.eval_gconfig.sampling_seed.
| if ( | |
| self.gconfig.sampling_seed is not None | |
| and not self.sglang.enable_deterministic_inference | |
| ): | |
| warnings.warn( | |
| "gconfig.sampling_seed is set but sglang.enable_deterministic_inference " | |
| "is False: SGLang silently ignores per-request sampling_seed unless the " | |
| "server is launched with --enable-deterministic-inference. Rollouts will " | |
| "not be seeded as expected. (Not applicable if you're launching SGLang " | |
| "servers yourself outside this config, or using the vLLM backend, which " | |
| "rejects sampling_seed outright.)", | |
| UserWarning, | |
| stacklevel=2, | |
| ) | |
| if ( | |
| (self.gconfig.sampling_seed is not None or self.eval_gconfig.sampling_seed is not None) | |
| and not self.sglang.enable_deterministic_inference | |
| ): | |
| warnings.warn( | |
| "gconfig.sampling_seed or eval_gconfig.sampling_seed is set but sglang.enable_deterministic_inference " | |
| "is False: SGLang silently ignores per-request sampling_seed unless the " | |
| "server is launched with --enable-deterministic-inference. Rollouts will " | |
| "not be seeded as expected. (Not applicable if you're launching SGLang " | |
| "servers yourself outside this config, or using the vLLM backend, which " | |
| "rejects sampling_seed outright.)", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
There was a problem hiding this comment.
Fixed as suggested. Also left a comment noting eval_gconfig is currently only read for .n_samples elsewhere in the codebase, so eval_gconfig.sampling_seed doesn't reach SGLang yet either way, this check is honest about the flag mismatch, not evidence the field does anything.
| trial_name="trial", | ||
| gconfig=GenerationHyperparameters(sampling_seed=42), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Since eval_gconfig can also have sampling_seed set, we should add a test case to verify that PPOConfig warns when eval_gconfig.sampling_seed is set without deterministic inference enabled.
)
def test_ppo_config_warns_when_eval_sampling_seed_set_without_deterministic_inference():
with pytest.warns(UserWarning, match="sampling_seed is set but"):
PPOConfig(
experiment_name="exp",
trial_name="trial",
eval_gconfig=GenerationHyperparameters(sampling_seed=42),
)There was a problem hiding this comment.
Added, thanks: test_ppo_config_warns_when_eval_sampling_seed_set_without_deterministic_inference, plus a "does not warn when consistent" case for symmetry.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for a per-request sampling_seed parameter to enable replayable sampling on the SGLang backend, along with a new enable_deterministic_inference configuration option. It also implements validation warnings for misconfigured seeds and raises a NotImplementedError on the vLLM backend where the parameter is unsupported. The feedback highlights a potential AttributeError in areal/api/cli_args.py if self.sglang is None during configuration validation, and suggests a robust guard to prevent initialization crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if ( | ||
| self.gconfig.sampling_seed is not None | ||
| or self.eval_gconfig.sampling_seed is not None | ||
| ) and not self.sglang.enable_deterministic_inference: |
There was a problem hiding this comment.
If self.sglang is configured as null (which is common when using alternative backends like vLLM to avoid SGLang configuration overhead), accessing self.sglang.enable_deterministic_inference will raise an AttributeError. Guarding against None ensures the configuration initialization is robust and doesn't crash.
sglang_deterministic = (
self.sglang.enable_deterministic_inference
if self.sglang is not None
else False
)
if (
self.gconfig.sampling_seed is not None
or self.eval_gconfig.sampling_seed is not None
) and not sglang_deterministic:There was a problem hiding this comment.
It is reachable if PPOConfig is constructed directly in Python with sglang=None, bypassing the loader (dataclasses don't enforce type hints at runtime). Adding the guard and a test for that case specifically.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for a per-request sampling_seed parameter in the SGLang backend, which requires enabling the new enable_deterministic_inference flag. It also explicitly rejects sampling_seed with a NotImplementedError on the vLLM backend. The reviewer suggests validating the unsupported sampling_seed for the vLLM backend early during configuration initialization in PPOConfig.__post_init__ to prevent wasting resources before starting servers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if ( | ||
| self.gconfig.sampling_seed is not None | ||
| or self.eval_gconfig.sampling_seed is not None | ||
| ) and not sglang_deterministic: | ||
| warnings.warn( | ||
| "gconfig.sampling_seed or eval_gconfig.sampling_seed is set but " | ||
| "sglang.enable_deterministic_inference is False: SGLang silently " | ||
| "ignores per-request sampling_seed unless the server is launched " | ||
| "with --enable-deterministic-inference. Rollouts will not be seeded " | ||
| "as expected. (Not applicable if you're launching SGLang servers " | ||
| "yourself outside this config, or using the vLLM backend, which " | ||
| "rejects sampling_seed outright.)", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Instead of waiting for a runtime NotImplementedError when the first generation request is built, we should validate that sampling_seed is not used with the vllm backend during configuration initialization in PPOConfig.__post_init__. This prevents wasting time and resources (such as starting up servers and loading models) on an unsupported configuration.
rollout_backend = getattr(self.rollout, "backend", None) if self.rollout is not None else None
if rollout_backend is not None and rollout_backend is not MISSING and "vllm" in rollout_backend:
if (
self.gconfig.sampling_seed is not None
or self.eval_gconfig.sampling_seed is not None
):
raise ValueError(
"gconfig.sampling_seed or eval_gconfig.sampling_seed is set, "
"but the vLLM backend does not support sampling_seed."
)
if (
self.gconfig.sampling_seed is not None
or self.eval_gconfig.sampling_seed is not None
) and not sglang_deterministic:
warnings.warn(
"gconfig.sampling_seed or eval_gconfig.sampling_seed is set but "
"sglang.enable_deterministic_inference is False: SGLang silently "
"ignores per-request sampling_seed unless the server is launched "
"with --enable-deterministic-inference. Rollouts will not be seeded "
"as expected. (Not applicable if you're launching SGLang servers "
"yourself outside this config.)",
UserWarning,
stacklevel=2,
)There was a problem hiding this comment.
This is already handled — PPOConfig.post_init now raises a ValueError when rollout.backend is vLLM and sampling_seed is set (see the check immediately above this warning). This comment was made against an earlier revision of the diff, before that check was added. No further action needed here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a sampling_seed parameter to GenerationHyperparameters to support replayable sampling, along with a new enable_deterministic_inference flag for SGLang. It also adds validation to raise errors when sampling_seed is used with the unsupported vLLM backend, and includes comprehensive tests and documentation updates. The review feedback suggests mapping sampling_seed to seed for OpenAI-compatible backends instead of marking it as unsupported, and recommends removing unreachable mentions of vLLM from the SGLang warning message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "lora_name", # Not supported by OpenAI | ||
| "use_beam_search", # Not supported by OpenAI | ||
| "max_tokens", # deprecated by "completions", not used in "responses", should be `max_new_tokens` in "openai-agents" | ||
| "sampling_seed", # SGLang-specific; not an OpenAI-compatible parameter | ||
| } |
There was a problem hiding this comment.
OpenAI's Chat Completions and Completions APIs actually support a seed parameter for deterministic sampling. Instead of treating sampling_seed as unsupported by OpenAI, we can map it to seed in to_openai_args_dict so that OpenAI-compatible backends can also benefit from deterministic sampling.
To do this:
- Remove
"sampling_seed"from_OPENAI_UNSUPPORTED_ARGS(as suggested below). - Map
"sampling_seed"to"seed"into_openai_args_dict'smappingdictionary (around line 303):
mapping = {"n_samples": "n", "sampling_seed": "seed"} "lora_name", # Not supported by OpenAI
"use_beam_search", # Not supported by OpenAI
"max_tokens", # deprecated by "completions", not used in "responses", should be max_new_tokens in "openai-agents"
}There was a problem hiding this comment.
Thanks for the suggestion, but I don't think this is safe as proposed. to_openai_args_dict/to_openai_agents_model_settings_dict are outbound-only converters used to hand GenerationHyperparameters to the external openai-agents SDK, they're not part of AReaL's own request-building path. ArealOpenAI.create() builds a fresh GenerationHyperparameters from its own explicit kwargs and never reads sampling_seed/seed back in, so mapping it here wouldn't actually make AReaL's own rollouts seeded.
More importantly, if a mapped seed value ever reached a real OpenAI-compatible endpoint via the openai-agents SDK backed by vLLM, it would hit exactly the problem called out under "Not the vLLM path" in the PR description: vLLM's own per-request seed is stream-based and doesn't survive AReaL's interruption/resumption re-submits, so it wouldn't give the reproducibility guarantee the name implies. sampling_seed here is deliberately scoped to SGLang's own sampling_params['sampling_seed'] + --enable-deterministic-inference mechanism, which is why it's excluded rather than remapped. Happy to revisit once there's a validated vLLM-seed design.
| warnings.warn( | ||
| "gconfig.sampling_seed or eval_gconfig.sampling_seed is set but " | ||
| "sglang.enable_deterministic_inference is False: SGLang silently " | ||
| "ignores per-request sampling_seed unless the server is launched " | ||
| "with --enable-deterministic-inference. Rollouts will not be seeded " | ||
| "as expected. (Not applicable if you're launching SGLang servers " | ||
| "yourself outside this config, or using the vLLM backend, which " | ||
| "rejects sampling_seed outright.)", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Since the vLLM backend check above raises a ValueError and terminates execution immediately if sampling_seed is set, the warning's mention of or using the vLLM backend, which rejects sampling_seed outright is unreachable dead code. Removing this part makes the warning message cleaner and more accurate.
warnings.warn(
"gconfig.sampling_seed or eval_gconfig.sampling_seed is set but "
"sglang.enable_deterministic_inference is False: SGLang silently "
"ignores per-request sampling_seed unless the server is launched "
"with --enable-deterministic-inference. Rollouts will not be seeded "
"as expected. (Not applicable if you're launching SGLang servers "
"yourself outside this config.)",
UserWarning,
stacklevel=2,
)The deterministic-inference UserWarning only ever fires when rollout.backend is not vLLM: the ValueError check immediately above it already raises and aborts construction whenever both a vLLM backend and a sampling_seed are set, regardless of the SGLang deterministic-inference flag. The warning text's parenthetical "...or using the vLLM backend, which rejects sampling_seed outright" therefore described a state that can never co-occur with the warning itself, per gemini-code-assist feedback on PR areal-project#1524. Reworded to state that plainly instead of implying it's a live alternative case. Refs: areal-project#1523
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a sampling_seed parameter to GenerationHyperparameters for replayable sampling, along with an enable_deterministic_inference flag in SGLangConfig. It implements seed forwarding for the SGLang remote engine and bridge, while raising a NotImplementedError for the vLLM backend. Additionally, validation is added to PPOConfig to warn or fail early on misconfigurations. The reviewer suggests mapping sampling_seed to seed in to_openai_args_dict instead of marking it as unsupported, since OpenAI's API natively supports a seed parameter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "lora_name", # Not supported by OpenAI | ||
| "use_beam_search", # Not supported by OpenAI | ||
| "max_tokens", # deprecated by "completions", not used in "responses", should be `max_new_tokens` in "openai-agents" | ||
| "sampling_seed", # SGLang-specific; not an OpenAI-compatible parameter |
There was a problem hiding this comment.
OpenAI's Chat Completions and Completions APIs actually support a seed parameter (introduced in late 2023) to make a best effort to sample deterministically.
Instead of marking sampling_seed as an unsupported OpenAI argument, it would be highly beneficial to map it to seed in to_openai_args_dict so that any OpenAI-compatible backends can also leverage this parameter.
You can achieve this by:
- Removing
"sampling_seed"from_OPENAI_UNSUPPORTED_ARGS. - Adding
"sampling_seed": "seed"to themappingdictionary insideto_openai_args_dict.
There was a problem hiding this comment.
already replied above why this is not a good idea.
SushantDaga
changed the title
|
This pull request has been automatically marked as stale because it has not had recent activity within the last 14 days. Please add a comment or push new commits to keep it active. Thank you for your contribution! |
| if self.sglang is not None | ||
| else False | ||
| ) | ||
| if ( |
There was a problem hiding this comment.
I think there should be a check here: when the flag is enabled, no seed is set, and gconfig.n_samples > 1, all requests fall back to SGLang's default seed 42, sharing the same noise across prompts, leading to diversity collapse within the group.
There was a problem hiding this comment.
You're right, thanks. That confirms the open question at the end of the PR description. I've added the guard in PPOConfig.__post_init__ at this spot.
I made it a ValueError rather than a warning. My reasoning, and the alternative I weighed:
- This combination (
enable_deterministic_inferenceon,n_samples > 1, nosampling_seed) has no working use. The group's same-prompt rollouts collapse to identical completions and GRPO's advantage goes to zero, so it quietly defeats the flag's own purpose. enable_deterministic_inferenceis new in this PR and defaults to off, so failing fast can't break any existing config. It's the same fail-fast this PR already does forsampling_seedon the vLLM backend.- The alternative was a warning, which would match the existing
mean_level='group'+group_size=1check incli_args.py(that one warns for a similar group-quantity degeneracy). I went with raising because that check guards a pre-existing surface where a warning avoids breaking someone, whereas this flag doesn't exist until the PR merges, so there's nobody to break. Happy to switch it to a warning if you'd rather keep the two consistent. - It only fires for stochastic sampling. Under
greedyortemperature == 0the request builders decode at temperature 0.0, so the group collapses regardless of any seed, and the guard skips that case to avoid a misleading error. It also skips the vLLM backend, sinceenable_deterministic_inferenceis SGLang-only and inert on vLLM, so an SGLang-worded error there would just point someone at the wrong subsystem (it reuses the samerollout.backendparse the vLLM guard above already does).
Tests are in tests/test_sampling_seed_deterministic_inference_warning.py: it raises for grouped + deterministic + unseeded, and does not raise with a seed set, at n_samples == 1, under greedy or temperature == 0, or on a vLLM backend.
sitabulaixizawaluduo
left a comment
sitabulaixizawaluduo
left a comment
There was a problem hiding this comment.
Thank you for your contribution, please rebase the code and resolve the conflicts
dbd8156 to
203f376
Compare
The deterministic-inference UserWarning only ever fires when rollout.backend is not vLLM: the ValueError check immediately above it already raises and aborts construction whenever both a vLLM backend and a sampling_seed are set, regardless of the SGLang deterministic-inference flag. The warning text's parenthetical "...or using the vLLM backend, which rejects sampling_seed outright" therefore described a state that can never co-occur with the warning itself, per gemini-code-assist feedback on PR areal-project#1524. Reworded to state that plainly instead of implying it's a live alternative case. Refs: areal-project#1523 Signed-off-by: Sushant Daga <dsush2307@gmail.com>
|
@sitabulaixizawaluduo Thanks for the review! Done, rebased onto latest Plus, added DCO to earlier commits and this one as per contribution guideline. |
Add an opt-in, default-off sampling_seed field to GenerationHyperparameters and an enable_deterministic_inference flag to SGLangConfig, forwarded into SGLang's sampling_params by both the v1 (SGLangBackend) and v2 (SGLangBridgeBackend) request builders. SGLang already ships a production per-request seeded sampler (multinomial_with_seed) documented for RL/GRPO debugging; AReaL never sent it. Both vLLM request builders (VLLMBackend, VLLMBridgeBackend) raise NotImplementedError on sampling_seed rather than silently ignoring it, and PPOConfig.__post_init__ warns at config-construction time if the seed is set without the server flag. Scoped to primitive plumbing only. Per-rollout seed minting (threading a sample identity through RolloutWorkflow/GroupedRolloutWorkflow so a GRPO group's K samples get distinct seeds) is deferred as an open design question, not resolved in this PR; see the PR description for why, and for a known footgun if sampling_seed is set naively on a multi-sample workflow. 11 new tests; no regressions in the 261 pre-existing tests for touched files. docs/en|zh/cli_reference.md regenerated for the two new fields. Signed-off-by: Sushant Daga <dsush2307@gmail.com>
…ic-inference warning Gemini Code Assist flagged that PPOConfig.__post_init__'s new warning (added in b2895ae) only checked gconfig.sampling_seed, missing the case where eval_gconfig carries its own independent sampling_seed. Fixed as suggested. While verifying the fix, found that eval_gconfig is currently consumed nowhere in the codebase except rl_trainer.py reading its n_samples field; no stock eval workflow builds a request from eval_gconfig's other fields. So eval_gconfig.sampling_seed never reaches SGLang regardless of this warning. Added a code comment documenting this so "no warning fired" is never misread as "eval_gconfig.sampling_seed works" - the check is honest about the flag mismatch, not about the field being live. Two new tests for the eval_gconfig warning cases (5 total in the file now, 263 passing overall, no regressions). Signed-off-by: Sushant Daga <dsush2307@gmail.com>
…heck
Gemini Code Assist flagged that self.sglang.enable_deterministic_inference
would raise AttributeError if self.sglang were None. Verified: the YAML/CLI
config loader rejects `sglang: null` (OmegaConf ValidationError, field is
not Optional), so this is unreachable through any supported config-file
path. It is reachable via direct Python construction
(PPOConfig(sglang=None, ...)), which bypasses that loader and isn't
type-checked at runtime. Added the guard for that case, plus a test.
This guard is the only unconditional access point to config.sglang in the
codebase; every other consumer (areal/infra/launcher/{local,ray,slurm,
sglang_server}.py) gates access behind `alloc_mode.gen_backend ==
"sglang"`, so config.sglang is never dereferenced when the backend is
vLLM. Nothing downstream is silently broken for the realistic
sglang=None-with-vLLM case.
Signed-off-by: Sushant Daga <dsush2307@gmail.com>
Gemini Code Assist suggested validating this in PPOConfig.__post_init__
instead of waiting for the runtime NotImplementedError that
VLLMBackend/VLLMBridgeBackend already raise on the first generation
request, so an unsupported config fails before wasting time on server
launch and model load.
Applied with two changes from the suggested code: compare
rollout_backend.split(":")[0] == "vllm" instead of a substring check
(matches how ModelAllocation, the codebase's own parser, splits this
string, so a hypothetical future backend name containing "vllm" as a
substring would not false-positive), and isinstance(rollout_backend, str)
instead of comparing against the omegaconf MISSING sentinel directly
(MISSING is literally the string "???", so this also covers any other
non-string case without needing that import).
Verified rollout.backend is the current, authoritative field (not the
deprecated top-level allocation_mode) by tracing RolloutController.__init__,
which parses config.backend where config is the InferenceEngineConfig.
Five new tests: vLLM+seed raises, vLLM-without-seed does not, SGLang+seed
does not (goes through the existing warning path instead), and unset
backend (the omegaconf MISSING default) does not crash or misfire as vLLM.
Signed-off-by: Sushant Daga <dsush2307@gmail.com>
…d guard
PPOConfig.__post_init__'s new vLLM+sampling_seed check only matched
plain "vllm:dims" backend strings via a bare split(":")[0], missing
the "vllm[name]:dims" form that RolloutController.__init__ actually
accepts (ModelAllocation.from_str's grammar allows an optional
"[name]" between backend and dims). A vLLM rollout configured with
a named backend and a sampling_seed would have silently skipped
this validation.
Also correct two comments found during self-review via /review-pr:
the claimed symmetry between the self.rollout and self.sglang
None-guards isn't real (self.rollout is never None in practice here,
since the use_lora check above already dereferences it unconditionally),
and both vLLM builders' "fail loudly" claim held only for
construction-time validation -- a seed set dynamically per-request
on the live rollout path still raises, but is caught by a generic
exception handler upstream and surfaces as a silently-rejected
rollout rather than a crash.
Key changes:
- Strip an optional "[name]" from rollout.backend before comparing
to "vllm" (areal/api/cli_args.py)
- Fix the misleading rollout/sglang guard-symmetry comment
- Fix VLLMBackend's incorrect same-file "below" reference and note
the construction-time-only scope of the fail-loud guarantee in
both vLLM builders (v1 and v2)
Refs: areal-project#1523
Signed-off-by: Sushant Daga <dsush2307@gmail.com>
The deterministic-inference UserWarning only ever fires when rollout.backend is not vLLM: the ValueError check immediately above it already raises and aborts construction whenever both a vLLM backend and a sampling_seed are set, regardless of the SGLang deterministic-inference flag. The warning text's parenthetical "...or using the vLLM backend, which rejects sampling_seed outright" therefore described a state that can never co-occur with the warning itself, per gemini-code-assist feedback on PR areal-project#1524. Reworded to state that plainly instead of implying it's a live alternative case. Refs: areal-project#1523 Signed-off-by: Sushant Daga <dsush2307@gmail.com>
PPOConfig.__post_init__ already warns when sampling_seed is set without enable_deterministic_inference. A reviewer flagged the inverse footgun on the PR: with enable_deterministic_inference on, no sampling_seed set, and gconfig.n_samples > 1, SGLang gives every seedless request the same default seed. Its noise is a pure function of (seed, position, vocab-index), so a group's same-prompt rollouts collapse to identical completions and zero out GRPO's advantage. Raise a ValueError rather than warn. Unlike the seed-ignored case above (harmless -- the seed is just dropped), this combination has no working use, and enable_deterministic_inference is new in this PR and default-off, so failing fast breaks no existing config -- the same fail-fast this PR already applies to sampling_seed on the vLLM backend. (A warning would instead be consistent with the existing mean_level='group' + group_size=1 singleton- group check, which warns for a similar degeneracy; that check guards a pre-existing config surface where raising could break someone, which this new default-off flag does not.) Scoped to stochastic sampling: under greedy or temperature == 0 the request builders decode at temperature 0.0, so the group collapses regardless of any seed and a per-request seed would not help. Also scoped to a non-vLLM backend, since enable_deterministic_inference is an SGLang-only flag that is inert on vLLM and this SGLang-worded error would otherwise misdirect a vLLM user (reusing the rollout_backend parse the vLLM guard already does). Six new tests: raises when grouped + deterministic + unseeded; does not raise with a seed, at n_samples == 1, under greedy / temperature == 0, or on a vLLM backend. Also regenerates docs/en|zh/cli_reference.md for the two fields (the rebase had reset them to upstream during conflict resolution). Signed-off-by: Sushant Daga <dsush2307@gmail.com>
203f376 to
4eb5798
Compare
|
@sitabulaixizawaluduo I have noticed that #1607 touches the same five files this PR does. They seems to be different change than this PR, but it will require a yet another rebase if that lands before merging. |
|
You can close this as its superseded by #1625 Two things:
Also, two things you might want to look at in current main: a seed set without |
2 participants
Description short
Add an opt-in, default-off sampling_seed field to GenerationHyperparameters and an enable_deterministic_inference flag to SGLangConfig, forwarded into SGLang's sampling_params by both the v1 (SGLangBackend) and v2 (SGLangBridgeBackend) request builders. SGLang already ships a production per-request seeded sampler (multinomial_with_seed) documented for RL/GRPO debugging; AReaL never sent it.
Both vLLM request builders (VLLMBackend, VLLMBridgeBackend) raise NotImplementedError on sampling_seed rather than silently ignoring it, and PPOConfig.post_init warns at config-construction time if the seed is set without the server flag.
Scoped to primitive plumbing only. Per-rollout seed minting (threading a sample identity through RolloutWorkflow/GroupedRolloutWorkflow so a GRPO group's K samples get distinct seeds) is deferred as an open design question, not resolved in this PR; see the PR description for why, and for a known footgun if sampling_seed is set naively on a multi-sample workflow.
11 new tests; no regressions in the 261 pre-existing tests for touched files. docs/en|zh/cli_reference.md regenerated for the two new fields.
Related Issue
#1523
Description, detailed
This PR plumbs SGLang's existing per-request
sampling_seedprimitive through AReaL's rollout path.SGLang's inference server (AReaL's default backend) already ships a production per-request seeded sampler (
multinomial_with_seed: murmur-hashed (seed, position, vocab-index) → Gumbel → argmax), documented by SGLang specifically for RL/GRPO debugging ("diverse yet reproducible responses"; docs.sglang.io/advanced_features/deterministic_inference.html). It's gated on a per-requestsampling_params["sampling_seed"]plus the server flag--enable-deterministic-inference. AReaL currently sends neither:sample_paramsinbuild_generation_requesthas no seed key, andGenerationHyperparametershas no seed field. This PR is just the plumbing so the primitive is reachable from AReaL configs.Small, additive, default-off changes; when unset, no request, config, or behavior changes in any way:
GenerationHyperparameters.sampling_seed: int | None = None(areal/api/cli_args.py), a new optional per-request field, added to_OPENAI_UNSUPPORTED_ARGSsince it has no OpenAI-API equivalent.SGLangConfig.enable_deterministic_inference: bool = False(areal/api/cli_args.py), flows through the existingconf_as_dict()to CLI-flag passthrough with no further wiring; matches SGLang's ownServerArgs.enable_deterministic_inferencefield name.gconfig.sampling_seedintosampling_params["sampling_seed"]when set:SGLangBackend.build_generation_request(areal/engine/sglang_remote.py, the v1 remote-engine path) andSGLangBridgeBackend.build_generation_request(areal/v2/inference_service/sglang/bridge.py, the v2 data-proxy path, which mirrors the v1 backend by its own docstring and would otherwise have silently diverged).NotImplementedErrorifsampling_seedis set:VLLMBackend.build_generation_request(areal/engine/vllm_remote.py) andVLLMBridgeBackend.build_generation_request(areal/v2/inference_service/vllm/bridge.py). There's no vLLM wiring in this PR (see "Not the vLLM path" below), so failing loudly here matches howSGLangBackendalready rejects its own unsupporteduse_beam_search. That's better than silently no-op'ing and leaving a caller believing their vLLM rollouts are seeded when they aren't.PPOConfig.__post_init__(areal/api/cli_args.py) warns ifgconfig.sampling_seedis set butsglang.enable_deterministic_inferenceis False; see "No request-time warning" below for why this only catches the common case, not every deployment shape.docs/en/cli_reference.md/docs/zh/cli_reference.mdregenerated to include both new fields (uv run python docs/generate_cli_docs.py).That's it. This PR does not touch workflows, the training loop, schedulers, or weight updates.
Known limitation, please read before trying this on a GRPO group: this field has no in-repo consumer yet, and its one obvious naive use is a footgun worth stating plainly rather than leaving for someone to discover the hard way. AReaL's inference engine rejects
n_samples > 1outright (RemoteInfEngine.agenerate, "Inference engines do not support n_samples > 1. Please call generate multiple times with n_samples = 1."), so a GRPO group's K samples of one prompt are produced by callinggenerateK times, each building its request fromself.gconfig.new(n_samples=1)(e.g.RLVRWorkflow.arun_episode,VisionRLVRWorkflow.arun_episode,MultiTurnWorkflow.arun_episode)..new()isasdict(self)plus overrides, so if a caller setssampling_seedonce on a workflow'sgconfig, every one of that group's K rollouts inherits the identical seed. Withenable_deterministic_inference=Trueand an identical prompt, SGLang's seeded sampler is then a pure function of (seed, position, vocab) for all K calls, so the group collapses to byte-identical completions and GRPO's advantage degenerates to zero.This is exactly the failure mode the deferred per-rollout seed-minting (below) exists to prevent. Read the field as "the wire," not yet "a feature you can turn on for GRPO today." Until group-aware seed minting lands (in this PR or a follow-up, your call), the safe use is a fixed seed across a batch of distinct prompts (e.g. a held-out eval set), not repeated same-prompt samples.
What this deliberately does NOT do (open questions, not resolved here):
H(master_seed, prompt_hash, group_index)) so a GRPO group's K samples of one prompt get diverse-but-reproducible seeds instead of colliding on SGLang's default seed (worth flagging: under--enable-deterministic-inference, SGLang assigns seed 42 to any request that doesn't carry its own, seesampling_batch_info.py, so same-prompt group members would otherwise share identical noise). Doing this generically means threading a distinguishing index throughGroupedRolloutWorkflow.arun_episodeand by extension theRolloutWorkflowABC (areal/api/workflow_api.py), which ~9 concrete workflows implement, including experimental ones. That's a design/API decision, not something this PR should decide unilaterally. Happy to build it however you'd place it (kwarg, contextvar, data field) once there's agreement.sampling_seedbeside the per-tokenversionsAReaL already stores (ModelResponse.output_versions) would give a rollout a full replay coordinate. Deferred for the same reason as above; it depends on where minting lands.sampling_seedis set but the server flag is off.SGLangBackendis stateless (no config reference) andSGLangConfig(launch-time, server-side) isn't available wherebuild_generation_requestruns (client-side, and can legitimately be a different process from whatever launched the server); wiring a check there needs new cross-process config plumbing, out of scope here. What this PR does add:PPOConfig.__post_init__warns at config-construction time ifgconfig.sampling_seedis set butsglang.enable_deterministic_inferenceis False, since both fields live on the same top-level config object there (mirrors the existingreward_norm/eval_gconfigcross-field checks already in__post_init__), so it catches the common case (AReaL launches the SGLang server itself from this same config) without needing the cross-process plumbing. It won't catch the case where someone points AReaL at an SGLang server they launched separately.seedis stream-based and doesn't survive AReaL's interruption/resumption (which re-submits prompt plus generated-so-far as a new request, restarting the stream and misaligning the noise). We don't have a validated design for vLLM yet; happy to discuss if there's interest. Both vLLM builders raiseNotImplementedErroronsampling_seedfor now rather than staying silent.A question: under
--enable-deterministic-inference, SGLang assignssampling_seed=42to any request that doesn't carry its own. Since the noise is a pure function of (seed, position, vocab-index), doesn't that mean a GRPO group's same-prompt members all get identical noise today if this flag is ever turned on, collapsing group diversity? If so, per-rollout seeds aren't just a debugging nicety, they're a prerequisite for using this flag with GRPO at all. Happy to be corrected if I'm misreadingsampling_batch_info.py.Type of Change
Checklist
pre-commit run --files <touched files>; ran on every file this PR touches)docs/en/cli_reference.md/docs/zh/cli_reference.mdregenerated viadocs/generate_cli_docs.py)main/review-prcommand (ran, findings addressed in follow-up commits)/create-prAdditional Context
Update: Ran
/review-pr(this repo's project-specific review) plus addressed twogemini-code-assistfindings -- fixed a backend-prefix parsing gap that missedbracket-named vLLM backends (
vllm[name]:dims), corrected a couple of misleadingcomments, and removed dead text from the deterministic-inference warning message.
Tests: eleven new unit tests, pure config/dict assertions, no GPU, no live SGLang/vLLM server:
tests/test_sglang_generation_request.py(new, 2 tests):sampling_seedreachessample_paramswhen set onSGLangBackend, is absent whenNone.tests/test_sglang_bridge_generation_request.py(new, 2 tests): same, forSGLangBridgeBackend(the v2 path).tests/test_sglang_deterministic_inference_flag.py(new, 2 tests):enable_deterministic_inference=Trueproduces--enable-deterministic-inferencevia the realSGLangConfig.build_cmd/build_argspath;False(default) omits it. (build_argsasserts the installedsglangpackage version, which isn't installed on a non-GPU dev machine; the test monkeypatches that one check and exercises the actual production code path otherwise.)tests/test_vllm_generation_request.py(extended, +1 test):VLLMBackendraisesNotImplementedErrorwhensampling_seedis set.tests/v2/inference_service/test_inf_bridge.py(extended, +1 test): same, forVLLMBridgeBackend.tests/test_sampling_seed_deterministic_inference_warning.py(new, 3 tests):PPOConfigwarns whensampling_seedis set withoutenable_deterministic_inference, and doesn't warn when they're consistent or the seed is unset.All 11 new tests pass locally, alongside the full pre-existing suites for every touched file (261 total, no regressions):