fix(frontend): use --model-path metadata instead of worker MDC paths - #9110
fix(frontend): use --model-path metadata instead of worker MDC paths#9110Pernekhan wants to merge 2 commits into
Conversation
|
👋 Hi Pernekhan! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
477cd01 to
ae0ea91
Compare
f488286 to
88c3234
Compare
WalkthroughThe PR adds support for per-watcher local card overrides in the model discovery system. ChangesModel Card Override & Watcher Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
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 `@lib/llm/src/model_card.rs`:
- Around line 601-609: override_files_with currently updates
model_info/tokenizer/prompt_formatter/chat_template_file/gen_config but does not
clear the cached checksum used by mdcsum(), causing stale checksum reads; after
cloning those fields in override_files_with, reset the OnceLock that caches the
mdcsum (the cached checksum field used by mdcsum, e.g. mdc_sum or similar) so
subsequent calls to mdcsum() recompute the checksum; ensure you reference the
cached checksum field by its actual name and clear or reinitialize it inside
override_files_with immediately after the clones.
🪄 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: Pro
Run ID: b189791e-cd25-4ae5-ba37-71cbae2edfe3
📒 Files selected for processing (5)
lib/llm/src/discovery/watcher.rslib/llm/src/entrypoint/input/grpc.rslib/llm/src/entrypoint/input/http.rslib/llm/src/model_card.rslib/llm/tests/model_card.rs
88c3234 to
67cba1c
Compare
|
Both review comments addressed in 67cba1c:
Local checks still green: |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
8236247 to
ec710a7
Compare
|
Pushed ec710a7: gated the override on display_name match. Caught a multi-model edge case: if the frontend was launched with Cases:
Local checks still green: |
204374b to
312f07c
Compare
312f07c to
bf15c5a
Compare
When a worker publishes its MDC to discovery, the file paths inside it (tokenizer.json, config.json, etc.) point at the worker's local filesystem (e.g. /worker/weights). On the frontend node these paths typically don't exist, so download_config falls through to a HuggingFace Hub fetch using the worker's local path as if it were a repo id — producing a 401 because /worker/weights is not a valid HF id. Fix: when the frontend was started with --model-path /frontend/config, treat that LocalModel card as a metadata donor. The discovery watcher swaps the worker MDC's metadata-file slots (model_info, tokenizer, prompt_formatter, chat_template_file, gen_config) with the donor's references before download_config runs. download_config then takes the already-local fast path. This supports deployments where the worker has engine weights at one path and the frontend has a metadata-only directory (no weights) at a different path on a different filesystem. No new CLI flag — if --model-path was not passed, behavior is unchanged. Slot replacement is unconditional (no checksum gating). The frontend's --model-path is treated as the source of truth for tokenizer/config; the files at the two locations are expected to differ in general (e.g. weights present on worker, absent on frontend). Wired through both http.rs and grpc.rs run_watcher entry points. 4 new unit tests in lib/llm/tests/model_card.rs cover has_metadata_files and adopt_metadata_from (full slot replacement, runtime-field preservation, absent donor slots are skipped). Signed-off-by: Pernekhan Utemuratov <pernekhan@deepinfra.com>
bf15c5a to
50881fe
Compare
|
Heads-up — PR #9057 (frontend MDC verify-and-cache pipeline) is landing the same What the consolidation looks like: Final fallback chain in
What this means for your deployment: rung 3 fires, the operator's Multi-model robustness: because rung 3 only fires when Plumbing diff at consolidation time: drop Happy to loop you in on the consolidation when it goes up — if any of the rungs above don't match what your deployment needs, we can adjust. PR #9057: #9057 |
|
/ok to test |
@nnshah1, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
Threads the frontend's --model-path through ModelWatcher into checked_file_uri's fallback chain. When set, file:// (or path-only) CheckedFile slots whose worker-published location is unreachable on the frontend host fall back to <--model-path>/<basename> before hf://<source_path>/<basename>. Worker-published checksums stay authoritative; the override only chooses the byte source, not the expected hash, so any drift surfaces via blake3 verification. Coexists with PR #9110 (DeepInfra's wholesale-slot override): on merge, this URI-synthesis-time override supersedes override_files_with. Resolution chain in checked_file_uri: 1. http(s):// or hf:// -> use as-is 2. file:// AND original file exists -> use as-is 3. file:// AND original missing AND <--model-path>/<basename> exists -> rewrite to file://<--model-path>/<basename> 4. otherwise -> hf://<source_path>/<basename> Path-only CheckedFiles are coerced to a synthetic file:// URL up front so the four rungs apply uniformly. 9 unit tests cover all four rungs across path-only and url=file:// shapes. download_config gains an Option<&Path> parameter; ModelWatcher gains local_model_path field + set_local_model_path setter; entrypoints forward LocalModel.path() when --model-path was supplied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: nnshah1 <neelays@nvidia.com>
|
/ok to test 50881fe |
|
/ok to test 50881fe |
|
Heads-up — PR #9057 just merged (squash Marking this as draft for now since the capability is upstream — feel free to close it, or repurpose it for any follow-up gaps (e.g. multi-model display-name routing) you'd like to revisit. Thanks again for the contribution and for pushing on this case — the discussion here directly shaped the 4-rung design. |
Pull request was converted to draft
|
And to be clear — really appreciate you flagging the DeepInfra deployment topology and contributing the fix. That use case (frontend host has the model files at a different path than the worker advertises) wasn't on our radar, and your PR is what surfaced it. The merged version generalized the approach, but the feature is yours — thanks for pushing on this. 🙏 |
|
#9057 addresses what we wanted. Thanks for quickly shipping it. |
Overview:
We deploy the frontend and workers on separate hosts. Workers hold the engine weights at one path (say
/data/<model>on the GPU node), and the frontend only needs the small metadata files (config, tokenizer, chat template) at some path on the CPU node. Those two paths often don't match.Today the frontend just trusts the worker's MDC paths. If they don't resolve locally, it tries to grab the files from HuggingFace using the worker's local path as a repo id — which obviously can't work. There's no way to tell the frontend "use my own files instead."
This PR adds that. Pass
--model-pathto the frontend and its files take precedence over what the worker advertises. The worker still owns everything else (kv cache size, runtime config, etc.).Details:
In
do_worker_set_registration, beforedownload_config, we replace the worker MDC's metadata-file slots (model_info,tokenizer,prompt_formatter,chat_template_file,gen_config) with whatever the frontend's--model-pathloaded. Without--model-path, the watcher getsNoneand behaves exactly like main.Internal API:
ModelDeploymentCard::override_files_with(&mut self, other: &Self)ModelWatcher::set_local_card(Option<ModelDeploymentCard>)3 unit tests in
lib/llm/tests/model_card.rs. Existing tests still pass. 5 files changed, +80 / −1.There's a bigger plan in DEP #8749 (workers self-host metadata over HTTP). This PR is a smaller, complementary change for deployments that can already mount the metadata files into the frontend pod.
Where should the reviewer start?
lib/llm/src/model_card.rs—override_files_with, five lines.lib/llm/src/discovery/watcher.rs— the call site indo_worker_set_registration, just beforedownload_config.lib/llm/src/entrypoint/input/http.rsand…/grpc.rs— same one-liner deciding whether to forward the frontend's card.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Tests