-
Notifications
You must be signed in to change notification settings - Fork 582
feat(sglang): plumb per-request sampling_seed for replayable sampling #1524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4c4fdfd
7066422
f551860
7dba683
51c35ee
485146c
4eb5798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -256,6 +256,16 @@ class GenerationHyperparameters: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sampling_seed: int | None = field( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default=None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metadata={ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "help": "Per-request seed for replayable sampling. On the SGLang backend " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "this is forwarded as sampling_params['sampling_seed'], which SGLang only " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "honors when the server is launched with " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "SGLangConfig.enable_deterministic_inference=True; otherwise it is " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "silently ignored. None (default) sends no seed and changes nothing." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # NOTE: to add new parameters, please correctly handle them in the `to_openai_args_dict` method. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def new(self, **kwargs): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -303,6 +313,7 @@ def to_openai_agents_model_settings_dict( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
313
to
317
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpenAI's Chat Completions and Completions APIs actually support a To do this:
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"
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Workflow-layer flags, not generation arguments. Exclude silently from | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2140,6 +2151,10 @@ class SGLangConfig: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enable_memory_saver: bool = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allow_auto_truncate: bool = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attention_backend: str | None = "fa3" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Required for per-request GenerationHyperparameters.sampling_seed to be honored | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # (SGLang gates sampling_seed on this flag). Also enables SGLang's batch-invariant | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # kernels, at their documented throughput cost, which is why this is opt-in. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enable_deterministic_inference: bool = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enable_multimodal: bool = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sampling_backend: str | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context_length: int | None = 32768 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -3364,6 +3379,96 @@ def __post_init__(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the engine config. Single source of truth: gconfig.lora_name. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self.rollout.use_lora and not self.rollout.lora_name: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.rollout.lora_name = self.gconfig.lora_name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # vLLM has no sampling_seed support (both VLLMBackend and VLLMBridgeBackend | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # raise NotImplementedError on the first request); fail here instead, before | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # any server launch or model load wastes time on an unsupported config. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # rollout.backend is the current per-engine field, e.g. "vllm:d2t4" or the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # named form "vllm[name]:d2t4" -- confirmed authoritative via | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # RolloutController.__init__ parsing config.backend through | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # ModelAllocation.from_str, whose grammar allows that optional "[name]", so | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # strip it before comparing rather than only splitting on ":". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Unlike self.sglang below, self.rollout is never None in practice here (the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # `self.rollout.use_lora` check above already dereferences it unconditionally), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # so this ternary is defense-in-depth, not a live guard. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rollout_backend = self.rollout.backend if self.rollout is not None else None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(rollout_backend, str) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and rollout_backend.split(":")[0].split("[")[0] == "vllm" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "rollout.backend is vLLM, which does not support sampling_seed." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # NOTE: eval_gconfig is currently only consumed for its n_samples field | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # (areal/trainer/rl_trainer.py); no stock eval workflow builds a request from | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # eval_gconfig's other fields, so eval_gconfig.sampling_seed does not reach | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # SGLang either way. Checked here anyway so this warning stays correct if that | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # changes, but do not read "no warning" as "eval_gconfig.sampling_seed works." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # self.sglang is not Optional and the YAML/CLI config loader (OmegaConf | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # structured-config validation) rejects `sglang: null`, but direct Python | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # construction (PPOConfig(sglang=None, ...), bypassing that loader) is not | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # type-checked at runtime, so guard rather than assume non-None here. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sglang_deterministic = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.sglang.enable_deterministic_inference | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self.sglang is not None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, thanks. That confirms the open question at the end of the PR description. I've added the guard in I made it a
Tests are in |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 -- the vLLM case is already rejected " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "above, before this warning can be reached.)", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UserWarning, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stacklevel=2, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3420
to
+3434
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The warning only checks
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Comment on lines
+3420
to
+3434
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of waiting for a runtime 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,
)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Comment on lines
+3424
to
+3434
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the vLLM backend check above raises a 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,
) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Complementary to the warning above (the two never both fire: that one needs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the flag off, this one needs it on). With deterministic inference ON but no | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # per-request seed and n_samples > 1, SGLang gives every seedless request the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # same default seed (42, see sampling_batch_info.py in sglang); 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 rather than warn (unlike the seed-ignored case above, which is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # harmless): the combination has no working use, and enable_deterministic_ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # inference is new here and default-off, so failing fast breaks no existing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # config -- the same fail-fast this class already applies to sampling_seed on | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the vLLM backend. Scoped to stochastic sampling: under greedy or | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # temperature == 0 the request builders decode with temperature 0.0 (see | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # "0.0 if gconfig.greedy else gconfig.temperature" in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # SGLangBackend.build_generation_request), so the group collapses regardless of | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # any seed and a per-request seed would not change it. Also scoped to a non-vLLM | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # backend: enable_deterministic_inference is an SGLang-only flag, so on a vLLM | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # run it is inert and this SGLang-worded error would misdirect debugging. The | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # vLLM guard above only fires when a seed is set, so it never covers this | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # unset-seed case; scope it out here, reusing the rollout_backend parsed above. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sglang_deterministic | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and self.gconfig.sampling_seed is None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and self.gconfig.n_samples > 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and not self.gconfig.greedy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and self.gconfig.temperature > 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and not ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(rollout_backend, str) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and rollout_backend.split(":")[0].split("[")[0] == "vllm" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sglang.enable_deterministic_inference is True with gconfig.n_samples " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "> 1 but no gconfig.sampling_seed: SGLang gives every seedless request " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "the same default seed, so the group's same-prompt rollouts collapse to " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "identical completions. Set distinct per-rollout seeds, use " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "n_samples=1, or disable enable_deterministic_inference." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| super().__post_init__() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenAI's Chat Completions and Completions APIs actually support a
seedparameter (introduced in late 2023) to make a best effort to sample deterministically.Instead of marking
sampling_seedas an unsupported OpenAI argument, it would be highly beneficial to map it toseedinto_openai_args_dictso that any OpenAI-compatible backends can also leverage this parameter.You can achieve this by:
"sampling_seed"from_OPENAI_UNSUPPORTED_ARGS."sampling_seed": "seed"to themappingdictionary insideto_openai_args_dict.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already replied above why this is not a good idea.