fix(frontend): reconcile model registrations - #11092
Conversation
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
This comment has been minimized.
This comment has been minimized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds periodic reconciliation to the discovery watcher, retries incomplete model registrations, removes stale local cards, and adds a key accessor plus a completeness test. ChangesDiscovery reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lib/llm/src/discovery/watcher.rs`:
- Around line 340-341: The reconciliation path in DiscoveryWatcher is blocking
the watch loop because the reconciliation tick awaits `reconcile` inline and
stale cleanup in `handle_delete` can also stall on in-flight work. Refactor the
`reconciliation.tick()` branch and the stale-entry cleanup path so discovery
event processing in the watch loop stays responsive, for example by moving
`reconcile` to a single-flight background task or making cleanup
incremental/non-blocking while preserving the existing `reconcile` and
`handle_delete` behavior.
- Around line 119-125: The registration-complete check in watcher.rs is only
verifying that a WorkerSet exists, so it can incorrectly treat a stale or
mismatched local registration as complete. Update the condition built around
get_model_card, get_model, and has_worker_set in the reconciliation logic to
also require WorkerSet checksum compatibility with the saved card mdcsum before
returning true. Use the existing ws_key path and the model/card lookup flow to
compare the stored WorkerSet metadata against the current card checksum, so
retry_model_registration still runs when the local state is out of sync.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 803a19d2-babc-4a87-bbad-5fcbc5fcd110
📒 Files selected for processing (2)
lib/llm/src/discovery/model_manager.rslib/llm/src/discovery/watcher.rs
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Overview:
Add periodic, source-of-truth reconciliation so a frontend eventually rebuilds its local model registry after missed discovery events or transient registration failures.
The scope follows Neelay Shah’s later clarification, recorded on issue #10732:
/health,/live, and request-level process readiness remain unchanged, while the existing serving-readiness filter continues to exclude incomplete models fromGET /v1/models.Details:
DiscoveryQuery::AllModelsevery 30 seconds and process model-card identities deterministically./v1/modelsserving-readiness filter and leave frontend health and process-readiness semantics unchanged.Validation:
cargo fmt --all -- --checkcargo test -p dynamo-llm --no-default-features discovery::watcher::tests --lib— 12 passedcargo test -p dynamo-llm --no-default-features serving_ready_excludes_incomplete_namespace --lib— 1 passedcargo clippy -p dynamo-llm --no-default-features --lib -- -D warningsWhere should the reviewer start?
ModelWatcher::watchinlib/llm/src/discovery/watcher.rsfor the single-flight background scheduling.ModelWatcher::reconcileandhandle_deletein the same file for retry and stale-removal behavior.is_registration_completefor the card, WorkerSet, and checksum convergence condition.ModelManager::get_model_card_keysinlib/llm/src/discovery/model_manager.rsfor the reconciliation snapshot accessor.Related Issues
🔗 This PR is linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes