Skip to content

refactor(rl): derive use_rollout_sampling_mask once and thread it through - #2

Closed
guapisolo wants to merge 7 commits into
nanjiangwill:feat/top-p-sampling-mask-actor-logprobsfrom
radixark:jiajun/sampling-mask-sot-pr3354
Closed

guapisolo wants to merge 7 commits into
nanjiangwill:feat/top-p-sampling-mask-actor-logprobsfrom
radixark:jiajun/sampling-mask-sot-pr3354

Conversation

@guapisolo

Copy link
Copy Markdown

Summary

Derive use_rollout_sampling_mask once at validation and thread it to every replay consumer.

Motivation

Sampling-support replay was switched on by calling sampling_support_replay_enabled(args) at every consumer: the Megatron and FSDP actor gates, policy_loss_function, should_return_sampling_mask, the agentic hub, OpenAIEndpointTracer.create, both session cores, resolve_request_args_by_config, and argument validation. Each site re-derived the same predicate from rollout_top_p / rollout_top_k through getattr defaults, so any object lacking those fields silently evaluated to "replay off", and SessionServerConfig had to duck-type three numeric fields to stand in for args. The sibling R3 switch already follows a different pattern: use_rollout_routing_replay is one value on args, copied into SessionServerConfig, and read directly everywhere. Public API, the activation condition (rollout_top_p < 1 or rollout_top_k > 0), the per-request checks, the actor-only gates, and the wire formats stay the same.

Before / After

  • Before / After: before, "is replay on" was a function evaluated independently at ten call sites with getattr fallbacks; after, miles_validate_args computes args.use_rollout_sampling_mask right after the top-p / top-k range checks, the fail-closed block keys on it, and every consumer reads the attribute directly.
  • What moved where: the predicate moved from miles/utils/sampling_mask.py (helper removed) into miles/utils/arguments.py, SessionServerConfig gained use_rollout_sampling_mask: bool filled from args next to use_rollout_routing_replay, and the consumers in backends/megatron_utils/{actor,model}.py, backends/fsdp_utils/actor.py, backends/training_utils/loss_hub/losses.py, rollout/generate_utils/{sampling_mask,openai_endpoint_utils}.py, rollout/generate_hub/agentic_tool_call.py, rollout/session/{core,request_args}.py, plus rollout/session/v2/core.py switched to direct attribute reads.
  • Kept: SessionServerConfig still carries rollout_top_p / rollout_top_k / rollout_temperature because they fill missing request parameters.
  • Failure mode: a missing attribute now raises AttributeError instead of silently disabling replay.

Behavior Preservation

  • tests/fast/utils/test_arguments.py::test_sampling_support_arguments_fail_closed: every launch-time rejection (top-p without top-k, prefill recompute, missing Miles router, KL / OPD) still fires from the same flags.
  • tests/fast/rollout/generate_utils/test_sampling_mask.py: request-level validation plus the return_sampling_mask payload decision are unchanged for bounded, unbounded, or evaluation requests.
  • tests/fast/backends/test_fsdp_train.py, tests/fast/backends/megatron_utils/test_shared_ppo_lifecycle.py: actor-only scoring gates still exclude the ref / teacher prefixes.
  • tests/fast/rollout/session/: session config, request-arg resolution, sample assembly, and wire allowlists behave as before (mask field only when replay is on).

Verification

  • tests/fast/utils/test_arguments.py::test_bounded_rollout_sampling_derives_use_rollout_sampling_mask (new): the attribute is absent after parsing, then equals the derived value after validation for bounded and unbounded launches.
  • tests/fast/rollout, tests/fast/backends, tests/fast/router, tests/fast/utils, tests/fast/ray/rollout, tests/fast/examples on a CPU-only image (tests/fast/tinker excluded, tinker module absent): 8418 passed, 0 failed, 39 skipped, 7 xfailed.
  • Two results are environmental, not caused by this change: tests/fast/router/test_sglang_anthropic_conversion.py fails at import on a missing SGLANG_SOURCE_ROOT variable, while tests/fast/utils/test_http_utils.py::TestWaitTcpReadyAsync::test_it_returns_once_the_port_accepts hangs on the box (deselected) and touches no modified code.
  • black 24.3.0, isort 5.13.2, ruff 0.14.7: clean on all changed files.

Review Focus

  • miles/utils/arguments.py / miles_validate_args: the derived attribute is set before the fail-closed block, before args is serialized to Ray actors, and before compute_session_server_config runs.
  • miles/rollout/session/config.py: SessionServerConfig is a strict pydantic model, so every constructor in tests, benchmarks, or e2e helpers must now pass use_rollout_sampling_mask.
  • Direct attribute reads replace getattr defaults on purpose, so a custom rollout function that builds its own args object must set the attribute.

🤖 Generated with Claude Code

nanjiangwill and others added 7 commits September 21, 2026 07:20
…ough

Sampling-support replay was switched on by re-evaluating
sampling_support_replay_enabled(args) at every consumer: the Megatron and
FSDP actor gates, the policy loss, the rollout request decision, the
session-server config and wire allowlists, and argument validation. Each
site re-derived the same predicate from rollout_top_p / rollout_top_k with
getattr defaults, so an object lacking those fields silently evaluated to
"replay off" and the session server had to duck-type three numeric fields.

Derive the switch exactly once in miles_validate_args, right after the
top-p / top-k range checks, as args.use_rollout_sampling_mask, and make the
existing fail-closed block key on it. Thread that value the way
use_rollout_routing_replay already flows: SessionServerConfig gains a
use_rollout_sampling_mask bool filled from args, and every consumer reads
the attribute directly. The activation condition, the per-request checks in
should_return_sampling_mask, the actor-only gates, and the wire formats are
unchanged; a missing attribute now raises instead of disabling replay.

The helper is removed. Test fixtures set the attribute explicitly, and the
derivation is covered at parse level in test_arguments.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@guapisolo
guapisolo marked this pull request as draft September 22, 2026 08:13
@guapisolo
guapisolo marked this pull request as ready for review September 22, 2026 09:09
@guapisolo
guapisolo marked this pull request as draft September 22, 2026 09:21
@guapisolo
guapisolo marked this pull request as ready for review September 22, 2026 09:31
@guapisolo
guapisolo marked this pull request as draft September 22, 2026 10:24
@nanjiangwill
nanjiangwill force-pushed the feat/top-p-sampling-mask-actor-logprobs branch from 9f68c61 to ad27242 Compare September 22, 2026 18:04
@guapisolo

Copy link
Copy Markdown
Author

your pr radixark#3354 already include some of this logic, will close this now

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.

2 participants