[serve] Gate rank-consistency check on replica membership changes - #64911
[serve] Gate rank-consistency check on replica membership changes#64911johntaylor-cell wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 7fa7c85. Configure here.
There was a problem hiding this comment.
Code Review
This pull request optimizes the deployment state machine by gating the expensive rank-consistency check so that it only runs when replica membership changes. The current implementation uses a fingerprint calculated via reduce and xor over replica IDs. The review feedback suggests a more robust and idiomatic Python approach: storing and comparing a set of active replica IDs instead of an integer fingerprint. This eliminates the risk of hash collisions, removes unnecessary imports, and improves performance by leveraging optimized set comparisons. Corresponding updates to the unit tests are also provided.
8f8e90c to
a4eb683
Compare
4381251 to
3bddcac
Compare
abrarsheikh
left a comment
There was a problem hiding this comment.
Attach the before/after CPU profile in the PR description.
QQ: does the issue go away if we flip the default for _fail_on_rank_error to true?
No, the flag only decides what happens after the pass runs. Skipping the work is what this PR does: gate the pass on the active replica-id set, since |
b8ad31c to
363755b
Compare
363755b to
1cce43f
Compare
1cce43f to
0c7ddeb
Compare
The rank-consistency pass is O(N) with heavy constants and used to run on every control-loop tick; at 10K+ replicas it monopolizes the controller loop. Rank consistency can only be violated by a membership change, so gate the pass on the active replica-id set and skip it while that set is unchanged. Two O(1) guards run first -- status must be HEALTHY and there must be no STARTING replicas -- which rule out the busiest ticks before paying for the replica-list copy. Rank failures are invariant violations rather than expected conditions, so the gate carries no machinery for them: a pass that runs caches its membership, and nothing special happens if the underlying rank op logged and returned a safe default. The gate's tests inject a counting rank manager at the same seam the fixture already uses for actor wrappers, so the rank logic under test stays real and only the call count is added. Signed-off-by: john.taylor <john.taylor@anyscale.com> Co-Authored-By: Claude <noreply@anthropic.com>
0c7ddeb to
036265b
Compare

[serve] Gate rank-consistency check on replica membership changes
Why
The Ray Serve controller runs on the head node, continuously reconciling deployments and
making autoscaling decisions.
check_rank_consistency_and_reassign_minimallyruns at thetail of every
DeploymentState.update()— once per deployment per control-loop tick,which at measured loop rates is several times a second.
The pass is O(N) in replicas with large constant factors: it materializes the active-key
set and the rank-key set, takes set differences both ways, copies the entire
_ranksdict, tallies per-rank counts across every active key, and sorts all rank values — then
the manager above it regroups replicas per node. In steady state every one of those
passes re-derives an identical answer. At 16K, py-spy showed it monopolizing the event
loop and starving handle-report ingest for 20 s+.
Ranks can only become inconsistent when replica membership changes, so gate the pass on
the active replica-id set: run it when the set differs from the last checked one, skip
when it does not. Transitions are unaffected — membership churn during rollouts and
autoscaling changes the set, so the pass still runs exactly when it can matter. The
guards that were already there (deployment not HEALTHY, or any STARTING replica) run
first, before the replica list is materialized.
RAY_SERVE_FAIL_ON_RANK_ERRORdefaults off, so in production the pass can log an errorand return a safe default. When that happens the membership is deliberately not
cached and the check is retried next tick, rather than latching a membership the pass
never validated. The error flag is read before the reassignment reconfigure, which calls
back into the rank manager and resets it.
The gate's own cost is the remaining O(N) here: building the id set measures 1.9 ms at
16,384 replicas, ~4.5% of the optimized loop. #64910, stacked on this PR, replaces the
set comparison with a
ReplicaStateContainermutation-version integer, which removes it.What
_maybe_check_rank_consistency().ran, skip it.
RAY_SERVE_FAIL_ON_RANK_ERRORis off by default, so the pass can catch an exception andreturn
[]; caching that would mean a stable-but-inconsistent deployment never retries.DeploymentRankManagernow records whether the last rank op errored.never while STARTING replicas exist (the node-migration case documented in the original
comment).
Why this is safe
Rank consistency can only be violated by a membership change — replicas added, removed, or
replaced — and every such change alters the active id set, so the pass still runs on
exactly the ticks where it can find work. Rank release and removal from
_replicashappenin the same
pop(states=[STOPPING])branch, so the id set and the rank table cannot driftapart across ticks. The per-node and node-rank passes group off
_replica_to_node, whichis only mutated by rank assign/recover/release, so a fixed replica-id set implies a fixed
node grouping.
Set comparison is exact — there is no fingerprint collision to reason about. The residual
cost is the O(N) set construction itself (~1.9 ms/tick at 16K replicas); the follow-up
#64910 replaces it with an exact
ReplicaStateContainermutation counter, making the gateO(1).
Results
controller CPU samples; report ingest resumes (stale-report drops stop).
352.6 ms → 42.9 ms.
Testing
membership; re-runs on membership change; a swallowed rank error is not cached and
retries next tick; skips entirely (without recording a membership) while STARTING
replicas exist. Two over a real
DeploymentRankManager(fail_on_rank_error=False)—the production default — covering that a swallowed error is not cached, and that the
error flag is read before
_reconfigure_replicas_with_new_rankscan clear it.test_deployment_state.pysuite green (237 tests).