Allow score centering through compatible SGLang routers

[Shi-Dong]

Summary

Remove the score-centering argument check that forces session rollouts with more than 20 candidates onto the deprecated Miles router. A compatible SGLang router can now forward the larger chat request. Update the contract test and document the required router behavior.

This PR is stacked on the score-centering documentation PR and depends on SGLang #41373. Existing SGLang router builds that cap chat top_logprobs at 20 still require a smaller candidate count; the documentation calls for a routed-request check before launch.

Verification

• The original implementation branch passed 44 focused pipeline tests. This stack-only change removes the same restriction and updates its direct contract test.
• git diff --check passes.
• The focused test could not run on this Mac because its Python environment lacks torch. CI should run it on the PR branch.

[claude]

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.}

[Shi-Dong]

@claude review always

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🔴 miles/utils/score_centering.py — Removing this check drops the only fail-fast guard for large score-centering candidate counts on an incompatible SGLang router, so misconfigured runs now silently corrupt training data instead of erroring at launch. If the installed router still caps chat top_logprobs at 20 (the doc at docs/examples/infra-features/score-centering.md now just asks users to "verify" this manually), append_score_centering_topk in miles/rollout/generate_utils/score_centering.py truncates entries to fewer than k with no length check in "selected" mode, and fills the rest of the row with -1/-inf, so score centering trains on silently incomplete candidate sets rather than failing loudly. …

  Why this was flagged

  …Fix: keep an argument- or response-time check that rejects (or detects) fewer than score_centering_top_k candidates for chat/session rollouts, not just documentation advice.

  Trigger: --use-session-server with default top_p/top_k, score_centering_top_k>20, use_miles_router=False, and an SGLang router build that still caps top_logprobs at 20 (pre sglang PR #41373). Previously validate_score_centering_args (miles/utils/score_centering.py, removed lines) raised ValueError at startup. Now nothing checks the router's actual capability. In miles/rollout/generate_utils/score_centering.py append_score_centering_topk, for sampling_logprobs_mode=='selected', 'entries = entries[:k]' just truncates; there is no check that len(entries)==k (unlike the support-mode branch, which does check 'len(entries) != len(token_ids)'). Rows short of k are padded with -1/-inf, so training silently proceeds on partial candidate sets with no error, only a documentation suggestion to manually verify before launch.

  Verification: normal — acknowledged in diff. Base branch fail-fast guard removed at miles/utils/score_centering.py (deleted lines 76-84 in the diff) previously raised ValueError at startup for exactly: score_centering_top_k>20, rollout_top_p==1.0, rollout_top_k==-1, use_session_server, and not use_miles_router — because the SGLang Rust router "caps OpenAI top_logprobs at 20" (the removed message's own…

• 🟡 docs/examples/infra-features/score-centering.md — This doc is generated from examples/infra_features/score_centering/README.md (see the file's own header), but the PR hand-edits only the generated doc's later paragraph (line 68) and never touches the README source or the doc's own earlier line 27, which still says "For session-server rollouts using more than 20 candidates, also set --use-miles-router." Readers now see contradictory guidance in the same page, and the next run of scripts/tools/sync_example_docs.py (or its --check mode) will detect drift or silently overwrite the line-68 fix with the stale README text. Fix: edit examples/infra_features/score_centering/README.md (the actual source) and regenerate the doc via the sync script, updating both the "Enable it" mention and the later paragraph so they agree.

  Why this was flagged

  A user reading only the "Enable it" section at docs/examples/infra-features/score-centering.md:27 still sees "more than 20 candidates, also set --use-miles-router", contradicting the updated line 68 saying a compatible SGLang router build now works without it. examples/infra_features/score_centering/README.md:24 and :66 (the declared single source of truth per its own header) were never updated at all, so the doc was hand-edited instead of the README, violating the sync script's invariant. Running scripts/tools/sync_example_docs.py --check (used in CI per the script's docstring) will fail or regenerate the doc from the stale README, reverting this PR's doc fix. No safeguard catches this because the diff only touches the generated file, not its source.

  Verification: normal. The doc is a generated mirror: docs/examples/infra-features/score-centering.md:4 says "Generated from examples/infra_features/score_centering/README.md by scripts/tools/sync_example_docs.py. Edit that README, not this file." The PR hand-edits only the generated doc (paragraph now at line 69) and never touches the README — `git diff 2860923 HEAD --…

[guapisolo]

cc cmt

This push will rewrite shi/score-centering-sglang-router-pr from 3df660e8df to 358acf26c0 (force-with-lease on 3df660e8df). Its single commit is rebased onto the rewritten #3626:

• the guard removal and its test are kept, now written against --rollout-top-logprobs-num;
• the docs sentence now says --rollout-top-logprobs-num 128 instead of the removed default k=128.

Verification: fast tests show no new failures against the rebased baseline.

[claude]

I reviewed this PR and didn't find any confirmed bugs, but a human look is still worthwhile: the change removes the only launch-time guard against a known SGLang router limitation and there's an unaddressed comment on the PR since the latest push.

What was reviewed:

• validate_score_centering_args: the check that raised when session rollouts requested >20 top_logprobs without --use-miles-router is deleted outright, with no automated replacement (only doc guidance to "verify" manually before launch).
• append_rollout_topk (rollout_topk.py): if a router still truncates candidates to 20, missing slots are silently padded with -1/-inf rather than raising, so a stale/older router build would degrade score-centering data quietly instead of failing fast at launch as before.
• Updated test and docs match the new intended behavior and are internally consistent with the code change.

Extended reasoning...

The diff (single commit 358acf2) touches only miles/utils/score_centering.py, its contract test, and docs, removing a validation branch that previously forced session rollouts with >20 candidate top_logprobs onto a deprecated router, and replacing it with documentation asking users to manually confirm router compatibility; there is no auth/crypto/injection surface, but it does affect training-data integrity since the downstream candidate-collection code (rollout_topk.py) silently pads missing candidate slots rather than erroring, so a stale router could now degrade results without any runtime signal. Decisive factors for deferring rather than approving: the PR depends on an unmerged upstream SGLang PR for the new behavior to be safe, the removed check is not replaced by any automated runtime verification, and the timeline shows a third-party comment from guapisolo posted after the latest commit push with no subsequent commit or reply addressing it, which per policy should block approval.

[guapisolo]

cc cmt

This push will rewrite shi/score-centering-sglang-router-pr from 358acf26c0 to 4653de336e (force-with-lease on 358acf26c0). The branch is rebased onto the updated #3626; its own commit does not change.

Verification: on the stack tip, the rollout, session, sessions-v2, arguments, train-data and score-centering test suites pass (1023 passed, 17 skipped); 84/84 native and session request/response cases are byte-identical to the previous stack tip.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[guapisolo]

cc cmt

This push will rewrite shi/score-centering-sglang-router-pr from 4653de336e to 074150741a (force-with-lease on 4653de336e). The stack is rebased onto main 3439ec751. Changes:

• New commit "Move the router note into the score-centering README source" (review 5327639894, r4139099117):
  • The router paragraph was only in the generated mirror, so sync_example_docs.py --check and tests/fast/doc/test_sync_example_docs.py failed, and the sync hook would restore the old --use-miles-router text.
  • The README source now carries the paragraph, scoped to unfiltered session rollouts.
  • It keeps the note that filtered rollouts send sampling_logprobs_mode instead of top_logprobs, which the router must forward.
  • The setup note no longer asks for --use-miles-router, and the mirror is regenerated.

Verification on the stack tip 074150741a: the full tests/fast suite (14,629 passed) has no failures beyond the 14 that main 3439ec751 also has on the same devbox (HF LoRA target checks, the secret-flag classification test, the actor RPC surface test, one launch-script test, and one test that needs SGLANG_SOURCE_ROOT). The tip also merges cleanly with the current main 5ebc06f8d2.

[claude]

I reviewed this PR and didn't find any bugs. Because these two new commits remove a code-level guardrail (the ValueError that previously required --use-miles-router for session rollouts with more than 20 candidates) and replace it with documentation-only guidance that depends on an unmerged upstream SGLang router change, a human look is still worthwhile.

What was reviewed:

• The removed check in miles/utils/score_centering.py and its replacement prose in both doc copies (docs/examples/infra-features/score-centering.md, examples/infra_features/score_centering/README.md).
• The updated contract test (test_large_session_heads_can_use_sglang_router), which now asserts success even with use_miles_router=False, matching the code change.
• No remaining code-level check catches a mismatch between an older/default SGLang router (which caps top_logprobs at 20) and a configured candidate count above 20; enforcement is now purely operator-driven per the docs.

Extended reasoning...

This is the first review pass over commits 503eae8 and 0741507 (pushed 2026-09-30), which is when the actual functional change landed: prior reviews of this PR (including mine) only saw the documentation groundwork, since the ValueError removal in miles/utils/score_centering.py did not exist in the tree until 503eae8. The change removes a code-enforced safety check tying score-centering session rollouts (>20 candidates) to --use-miles-router, replacing it with documentation asking the operator to manually verify SGLang router compatibility, and explicitly depends on an unmerged upstream SGLang PR (#41373). miles/utils/ is CODEOWNERS-covered by guapisolo (who left an inline comment on this exact doc line and commented again after the fix commits) and Shi-Dong (the author), so a domain reviewer is already engaged, but the removal of a runtime guardrail in favor of prose-only guidance is a design decision worth a human sign-off rather than automated approval.

[guapisolo]

codex cmt

This history rewrite from e990eaff3841cf0a5cb9d8a8c9b26bd75761ce1b to bdfdf7a7a6983f1013d76261dc27ab0918ec604b will replay the router compatibility changes onto the updated stack while retaining its own commits, incorporating the human-review improvements in math, candidate transport, training, and documentation. The combined stack passed 213 tests, including TP2/CP2 Gloo and real Ray transport, with 7 GPU-dependent skips.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• 🔴 miles/backends/training_utils/loss_hub/score_centering/estimator.py:61 — Removing the finite-advantage and non-finite-log-prob guards lets a NaN/Inf advantage or log-prob silently turn score_c…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[guapisolo]

codex cmt

This history rewrite from bdfdf7a7a6983f1013d76261dc27ab0918ec604b to af511b67a1253bbc386ecdf05f877a2a35147a7b will replay this PR onto the updated stack with typed score-centering inputs and metrics, adapting its existing finite-check removal to the new constructor while retaining the router and logprob-mode changes. The final stack passed 213 tests with 7 GPU-dependent skips and 72 exact baseline comparisons of losses, metrics, and gradients.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[guapisolo]

codex cmt

This history rewrite from af511b67a1253bbc386ecdf05f877a2a35147a7b to a198d09e4cbfb2874a3b06c0294ff3000a26b0fc will replay this PR onto the updated stack with all estimator arguments in ScoreCenteringInputs, retaining its router, finite-check removal and logprob-mode commits. The final stack passed 213 tests with 7 GPU-dependent skips and 144 exact baseline comparisons covering default and custom options, losses, metrics, and gradients.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[HJSang]

LGTM

[guapisolo]

codex cmt

This history rewrite from a198d09e4cbfb2874a3b06c0294ff3000a26b0fc to 8648624a37f82ab8853f787c5c54f0a49be8c5f2 will rebase the final PR onto its updated documentation parent and main 64772f50, retaining the router, finite-check removal and logprob-mode commits together with the new shared-training integration. The final stack passed 219 focused tests, including native-actor/FSDP checks, real Ray transport and TP2/CP2 Gloo, with 7 GPU-dependent skips; all 75 original commits retain their raw author and committer metadata.

[guapisolo]

codex cmt

This history rewrite from 8648624a37f82ab8853f787c5c54f0a49be8c5f2 to d497ef7e44d533415e730e5aead3b119a065d5fa will advance this PR onto main 53f5d94224b7fc584857eb2aba68ffdd364dc139 through the updated stack parent, incorporating the concurrent CUDA 12 build cleanup while retaining the score-centering commits and the verified shared-training adaptations. The new final stack passed 219 focused tests with 7 GPU-dependent skips; this replay had no conflicts and preserved all 77 source commits with their raw author and committer metadata.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[guapisolo]

codex cmt

[P2] Commit 1145f1e8a451435b469095e38caf64161b4a9b09 will align the two Helm snapshots with the launcher's existing SGLANG_RETURN_ORIGINAL_LOGPROB=0 setting for every loss, correcting the stale expectations that fail CPU CI. The candidate's snapshot and inference-spec tests passed locally (106 tests with Helm 4.2.3); the push will run the full CPU CI via the existing bare run-ci label.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.