rollout: consistent_hashing/manual routing for inference_rollout stack - #1703
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements sticky routing for the consistent_hashing router policy by generating a unique session_id for each sample and passing it as an X-SMG-Routing-Key header in generation requests. It also adds a check in the HTTP utility to ensure this header is present for relevant endpoints when consistent hashing is enabled. The review feedback suggests replacing an assert statement with a ValueError in the routing key validation check to prevent it from being bypassed when Python is run with optimizations.
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.
| assert headers is not None and any(k.lower() == _ROUTING_KEY_HEADER.lower() for k in headers), ( | ||
| f"consistent_hashing router policy requires an {_ROUTING_KEY_HEADER} header on {parts.path} requests, " | ||
| f"but none was attached (url={url}). Set sample.session_id before generating so the generate " | ||
| f"function can attach the routing key." | ||
| ) |
There was a problem hiding this comment.
According to the project guidelines, ValueError should be used instead of assert for validating function arguments. Using assert can be bypassed if Python is run with optimizations (-O), which would silently disable this critical routing check.
| assert headers is not None and any(k.lower() == _ROUTING_KEY_HEADER.lower() for k in headers), ( | |
| f"consistent_hashing router policy requires an {_ROUTING_KEY_HEADER} header on {parts.path} requests, " | |
| f"but none was attached (url={url}). Set sample.session_id before generating so the generate " | |
| f"function can attach the routing key." | |
| ) | |
| if headers is None or not any(k.lower() == _ROUTING_KEY_HEADER.lower() for k in headers): | |
| raise ValueError( | |
| f"consistent_hashing router policy requires an {_ROUTING_KEY_HEADER} header on {parts.path} requests, " | |
| f"but none was attached (url={url}). Set sample.session_id before generating so the generate " | |
| f"function can attach the routing key." | |
| ) |
References
- Use
ValueErrorinstead ofassertfor validating function or constructor arguments (such as checking for positive or non-negative values).
1bf4d11 to
48c6509
Compare
|
|
||
| def compute_routing_headers(args, sample: Sample) -> dict[str, str] | None: | ||
| if args.sglang_router_policy == "consistent_hashing" and sample.session_id: | ||
| return {"X-SMG-Routing-Key": sample.session_id} |
There was a problem hiding this comment.
To avoid confusion with session server's session id, could you also modify the sample.session_id to sample.sglang_router_session_id?
9122755 to
d79eddf
Compare
|
Good fix. |
d79eddf to
7daffd4
Compare
|
@guapisolo yes if the user is using the session server then it's perfect. But in case some users might vibe their own session server, and they might not know adding this key.. |
7daffd4 to
15d6bbe
Compare
|
Update: dropped the client-side check from this PR — keyless-request enforcement moved to the router, where it covers every client (session server, custom proxy layers, raw aiohttp callers) instead of only requests going through miles' |
15d6bbe to
d1cf52b
Compare
d1cf52b to
289ca4b
Compare
…ollout stack - rename Sample.session_id to Sample.routing_key to disambiguate from the session server's and p2p transfer engine's session ids - unify all routing-key sites behind policy_uses_routing_key, covering the manual policy (#1690) as well: both stacks' group assignment and eval, single_turn/multi_turn/legacy generate headers, prefill recompute - drop the MILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1 restriction on --sglang-router-policy Keyless-request enforcement lives router-side (radixark/sgl-router-for-miles#8), which covers every client including proxy layers that bypass miles' http utils.
289ca4b to
dd2a6cc
Compare
assert is stripped entirely under python -O / PYTHONOPTIMIZE=1, which would silently skip this check and reintroduce the keyless-request degradation this PR fixes. Enforcement now lives router-side too (sgl-router-for-miles#8), so this is defense-in-depth, but it should still fail loudly rather than be optimizable away.
Problem
The
consistent_hashingrouter policy needs every generation request to carry anX-SMG-Routing-Keyheader; a request without the key silently degrades — the router's implicit-key fallback can hash a constantauthorizationheader and pin all traffic to a single engine (the failure mode of #1657). The client half of this contract was only implemented in the legacy stack (#891):sglang_rollout.generate_and_rm_groupassigns per-sample keys andgenerate()attaches the header. The inference_rollout stack has neither — which is why--sglang-router-policywas hard-blocked underMILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1. The eval paths of both stacks also never assign keys, and #1690 is about to makemanuala second routing-key-based policy with its own scattered== "consistent_hashing"checks.Fix
Sample.session_id→Sample.routing_key(review feedback): the field is the per-sample routing key sent asX-SMG-Routing-Key, not a session — the new name disambiguates it from the session server's and the p2p transfer engine's session ids. Renamed repo-wide.policy_uses_routing_key(args)(consistent_hashingormanual) ingenerate_endpoint_utils, used by every key site — both stacks' group assignment and eval,compute_routing_headers, prefill-recompute batch gating and per-sample headers. The legacygenerate()and prefill per-sample path now reusecompute_routing_headersinstead of hand-building the header. This closes the coordination gap with [router] set manual policy (sticky + min_load) as default agentic routing policy #1690:manualgets keys on all paths, not just the two legacy-stack sites, and a future key-based policy is a one-line change.inference_rollout_common.generate_and_rm_groupassigns a per-samplerouting_key(mirrors the legacy stack);generate_hub.single_turn/generate_hub.multi_turnattach the header.eval_rollout_single_datasetmint an independent uuid per eval sample, so eval traffic spreads across engines while each sample stays pinned.MILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1restriction on--sglang-router-policy.Enforcement lives router-side: radixark/sgl-router-for-miles#8 rejects keyless requests with 400
missing_routing_keyunderconsistent_hashing/manualpolicies (escape hatch:--allow-requests-without-routing-key). The router is the only chokepoint that sees every client — including proxy layers and raw-aiohttp callers that bypass miles' http utils — so this PR carries no client-side check.Compat notes for the rename: pickled Samples in old partial-rollout buffers / debug dumps restore the stale
session_idattr; the new field defaults to None and gets a fresh routing key on the next submit — affinity resets once across restart, harmless. Out-of-repo code readingsample.session_idmust switch tosample.routing_key.Validation
policy_uses_routing_keytruth table (consistent_hashing/manual → true; None/cache_aware → false);compute_routing_headersattaches the header iff predicate + key set; batch prefill scoring disabled for both key policies;reset_for_retrypreservesrouting_key.pre-commit run --all-filespasses. Generate paths are unchanged when the policy is off (headers=Noneas before).🤖 Generated with Claude Code