fix(pd): Fix PD cache-aware policy lifecycle - #1520
Conversation
📝 WalkthroughWalkthroughAdds PolicyRegistry::remove_worker_from_pd_cache_aware to remove a worker from PD-mode cache-aware prefill/decode policies, and updates workflow steps to reinitialize PD cache-aware policies using current prefill/decode worker lists after worker lifecycle events. ChangesPD Cache-Aware Policy Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @aurickq, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to manage worker removal and initialization for Prefill-Decode (PD) cache-aware policies. Specifically, it adds a remove_worker_from_pd_cache_aware method to the PolicyRegistry and ensures that init_pd_cache_aware_policies is called during worker registration and removal workflows to maintain policy consistency. Unit tests have been included to verify the initialization and removal logic. The reviewer noted that integrating these calls into shared workflow steps correctly handles the global nature of PD policies and prevents code duplication.
| app_context | ||
| .policy_registry | ||
| .init_pd_cache_aware_policies(&prefill_workers, &decode_workers); |
There was a problem hiding this comment.
Calling init_pd_cache_aware_policies here ensures that PD policies are refreshed whenever a new worker is registered. Since this step is shared across different registration paths, it correctly handles the global nature of PD policies and avoids code duplication, ensuring consistent behavior for all worker types.
References
- If an optimization or logic is applicable to multiple code paths, extract it into a shared helper function to ensure consistency and avoid code duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/policies/registry.rs`:
- Around line 585-587: The test currently calls
registry.remove_worker_from_pd_cache_aware("http://prefill-1:8000") and
registry.remove_worker_from_pd_cache_aware("http://decode-1:8000") but never
asserts the observable routing/resulting state; update the PD cache-aware test
to validate post-removal behavior by invoking the routing/resolution method used
elsewhere (e.g., the registry's route selection or resolve_worker calls) and
assert that lookups no longer return the removed endpoints and that requests
route to expected remaining workers (or return an error/none when none remain);
use the same registry instance and the unique helper methods
remove_worker_from_pd_cache_aware and the registry's resolution method names to
locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cfb4078-047c-4803-958b-7e047ecc26ec
📒 Files selected for processing (5)
model_gateway/src/policies/registry.rsmodel_gateway/src/workflow/steps/local/remove_from_policy_registry.rsmodel_gateway/src/workflow/steps/local/update_policies_for_worker.rsmodel_gateway/src/workflow/steps/local/update_remaining_policies.rsmodel_gateway/src/workflow/steps/shared/update_policies.rs
|
thanks! |
Signed-off-by: Aurick Qiao <aurick@thinkingmachines.ai>
4134c6d to
02b3f79
Compare
Description
Problem
PD cache-aware routing uses dedicated prefill and decode policy instances, separate from the per-model policy used by the regular routing path. The regular path initializes and refreshes its cache-aware policy during worker registration, update, and removal, but the PD cache-aware policy instances were not refreshed through the same lifecycle hooks.
This shows up for PD configurations that use
cache_awarerouting for prefill and/or decode workers, especially when there is more than one worker in a PD role and requests have overlapping prefixes. The request path can reach a cache-aware policy whose string tree was never initialized, producing logs like:At that point, prefix-aware routing is bypassed for the affected PD policy and worker selection falls back to random selection. The symptom is that repeated or prefix-overlapping requests do not consistently preserve locality even though
cache_awarerouting is configured.Solution
Mirror the existing regular cache-aware lifecycle behavior for PD cache-aware policy instances:
This keeps the PD path aligned with the existing non-PD cache-aware path.
Changes
remove_worker_from_pd_cache_aware(...)next to the existing regular cache-aware removal helper.init_pd_cache_aware_policies(...)from worker registration, update, and post-removal policy refresh paths.Test Plan
Repro before this change:
cache_awareselected for prefill and/or decode routing and more than one worker in at least one PD role.Expected behavior after this change:
Commands run:
cargo fmt --all cargo test -p smg test_pd_cache_aware_policy_initialization cargo check -p smgcargo fmt --allcompleted successfully, while printing existing rustfmt warnings about unstable import-formatting options in the repo config.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Improvements
Tests