Skip to content

feat(self-host): flip DYN_SELF_HOST_METADATA default to ON (gh-8749) - #11417

Merged
rmccorm4 merged 6 commits into
mainfrom
nnshah1/gh-8749-default-on
Aug 3, 2026
Merged

feat(self-host): flip DYN_SELF_HOST_METADATA default to ON (gh-8749)#11417
rmccorm4 merged 6 commits into
mainfrom
nnshah1/gh-8749-default-on

Conversation

@nnshah1

@nnshah1 nnshah1 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Capstone of the gh-8749 stack: workers now advertise their MDC files over the system_status_server by default instead of requiring shared storage.

What changes

lib/llm/src/local_model.rs (single file, +12/-37 after squash):

  • self_host_metadata_default(None) returns true
  • Explicit falsy env values 0/false/no/off (case-insensitive, trimmed) opt out
  • Anything else stays on
  • Hard-fail (anyhow::bail!) at first registration when self_host_metadata=true but DYN_SYSTEM_PORT is unset — surfaces misconfig at startup instead of WARN spam plus silently degraded hf:// MDC advertisement

Behavior enumeration

Without self-host (DYN_SELF_HOST_METADATA=0)

  1. Worker downloads model from HF → local snapshot dir (whatever revision HF's main resolves to at that moment).
  2. Worker builds MDC with hf://<repo>/<filename> URLs and mdcsum = fold of (basename, blake3) over its resolved files.
  3. Worker publishes MDC via discovery.
  4. Frontend reads MDC from discovery.
  5. Frontend downloads independently from HF — may resolve a different revision if HF's main moved between the worker's download and the frontend's.
  6. Frontend blake3-verifies the downloaded files against MDC.
  7. Match → serve. Mismatch → hard error at startup, operator must intervene.

With self-host (default, DYN_SYSTEM_PORT=9090 set — operator-managed deployments)

  1. Worker downloads model from HF → local snapshot.
  2. Worker builds MDC and rewrites URLs to http://<worker>:9090/v1/metadata/<ns>/<component>/<endpoint>/<slug>/<suffix>/<filename> pointing at its own copy.
  3. Worker publishes MDC via discovery.
  4. Frontend reads MDC.
  5. Frontend fetches from the worker's HTTP endpoint — exact bytes the worker used to build the MDC.
  6. Frontend blake3-verifies → always matches.
  7. Frontend caches locally under ~/.cache/dynamo/mdc/by-slug/<slug>/<mdcsum>/; subsequent workers with the same mdcsum are cache hits.

With self-host on but DYN_SYSTEM_PORT unset (bare / non-operator upgrade)

Worker anyhow::bail!s at first model registration with an actionable message: set DYN_SYSTEM_PORT or opt out with DYN_SELF_HOST_METADATA=0.

HF revision-mismatch and a possible follow-up

The self-host path solves the HF-mismatch problem by construction — worker-served bytes are the source of truth. The non-self-host path detects mismatches (blake3 fails and the frontend errors) but doesn't converge on the same revision.

The hf:// URL format the worker rewrites into already supports a revision segment: parse_hf_uri in lib/llm/src/model_card.rs:640 handles hf://repo[@rev]/filename. The construction site (local_model.rs:619) currently builds hf://<source_path>/ without a revision, but the resolved snapshot's SHA is knowable (HF cache stores files under snapshots/<sha>/).

Embedding the resolved SHA in the hf:// URL — e.g. hf://Qwen/Qwen3-0.6B@abcdef.../config.json — would let the frontend pin its download to the same revision the worker used, eliminating the mismatch class entirely in the non-self-host path. Small change; out of scope for this PR but a natural follow-up if the opt-out path is expected to be used long-term.

Why now

All prerequisites are in main:

Opt-out path

Existing deployments that need to fall back to shared-storage MDC delivery can pin DYN_SELF_HOST_METADATA=0 in their pod spec. That gives operators a rollback lever without needing an image swap.

Test coverage — honest read

Existing e2e: tests/frontend/test_self_host_metadata.py::test_worker_serves_metadata_via_http — a single post_merge test that spins up a mocker with DYN_SELF_HOST_METADATA=true explicitly set, then curls the /v1/metadata/... route.

Gaps against this PR:

  • The existing test explicitly sets the env, so it doesn't validate the flipped default (would pass either way).
  • No test for the anyhow::bail! path when DYN_SYSTEM_PORT is unset.
  • No test for the opt-out path (=0).
  • The file's docstring flags follow-ups that never landed: frontend verify-and-cache assertions, multi-replica + LoRA cases.

Recommendation before final merge: drop the explicit DYN_SELF_HOST_METADATA=true from the existing test so it validates the default, and add one test for the bail! path. Both are small and belong in a follow-up commit on this PR.

Docs

Only code-facing documentation was touched (the doc comment on ENV_SELF_HOST_METADATA). No user-facing docs currently describe the two modes. If we want to broaden reach, options:

  1. Add a section to docs/reference/glossary.md explaining "self-hosted vs shared-storage MDC delivery."
  2. Add a release-note entry for the default flip.
  3. Update the operator's helm chart README to note DYN_SYSTEM_PORT is now required (it already sets 9090 by default).

Test plan

  • cargo test -p dynamo-llm --lib passes locally
  • cargo fmt --check / cargo clippy -p dynamo-llm --lib --no-deps clean
  • Follow-up commit: update existing e2e to validate default, add bail! test
  • CI e2e green

🤖 Generated with Claude Code

@datadog-official

This comment has been minimized.

@nnshah1
nnshah1 force-pushed the nnshah1/gh-8749-default-on branch from 2e5e383 to 1428f39 Compare July 8, 2026 23:07
@github-actions github-actions Bot added documentation Improvements or additions to documentation deployment::k8s Relates to dynamo deployment in kubernetes frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` actions labels Jul 8, 2026
@nnshah1
nnshah1 force-pushed the nnshah1/gh-8749-default-on branch from 1428f39 to fc16f80 Compare July 8, 2026 23:09
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@nnshah1
nnshah1 marked this pull request as ready for review July 8, 2026 23:14
@nnshah1
nnshah1 requested a review from a team July 8, 2026 23:14

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

Open in Devin Review

Comment thread lib/llm/src/local_model.rs
Comment thread lib/llm/src/local_model.rs Outdated
Comment thread lib/llm/src/local_model.rs Outdated
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The default behavior of LocalModelBuilder::self_host_metadata is flipped: metadata rewriting is now enabled by default unless DYN_SELF_HOST_METADATA is explicitly set to a falsy value. Documentation is updated accordingly. move_to_self_host now fails with an error instead of warning when DYN_SYSTEM_PORT is unset. An outdated unit test module is removed.

Changes

Self-host metadata default behavior change

Layer / File(s) Summary
Env default parsing and docs
lib/llm/src/local_model.rs
self_host_metadata_default now returns true unless the environment variable is explicitly set to a falsy value (0/false/no/off), reversing the prior truthy-allowlist logic; doc comments for the builder and its method are updated to reflect the default-on behavior.
Fail-fast on missing port and test cleanup
lib/llm/src/local_model.rs
move_to_self_host now uses anyhow::bail! to error out when DYN_SYSTEM_PORT is unset while rewriting is enabled, replacing the prior warn-and-continue behavior; the env_self_host_metadata_tests module testing the old parsing logic is removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed, but it misses the required Related Issues section and the reviewer-start guidance from the template. Add the required Related Issues section with the issue link/confirmation, and include a short "Where should reviewer start?" note.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: flipping the self-host metadata default on.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/local_model.rs`:
- Around line 49-55: Add table-driven test coverage for
self_host_metadata_default to lock in the default-on behavior. Create cases for
None, whitespace-padded and mixed-case falsey inputs ("0", "false", "no",
"off"), truthy inputs, and arbitrary strings, and assert the function returns
the expected bool for each. Place the tests near self_host_metadata_default so
future changes to the normalization logic in this helper are caught.
🪄 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: 81084b5e-4b8a-4b19-89c8-f34c09d99bf5

📥 Commits

Reviewing files that changed from the base of the PR and between 689372a and fc16f80.

📒 Files selected for processing (1)
  • lib/llm/src/local_model.rs

Comment thread lib/llm/src/local_model.rs
@nnshah1
nnshah1 force-pushed the nnshah1/gh-8749-default-on branch 2 times, most recently from b840ab4 to dcc89a0 Compare July 8, 2026 23:32
Capstone of the gh-8749 stack: workers advertise their MDC files over
the system_status_server by default instead of requiring shared
storage.

Behavior:
  - Unset env  -> ON (new default)
  - 0/false/no/off (case-insensitive, trimmed) -> OFF (opt out)
  - Otherwise -> ON

Hard-fail (anyhow::bail!) when self_host_metadata is on but
DYN_SYSTEM_PORT is unset — surfaces misconfigurations at first
registration instead of WARN spam plus silently degraded hf:// MDC
advertisement. The k8s operator sets DYN_SYSTEM_PORT=9090 (see
deploy/operator/internal/dynamo/component_worker.go), so operator-
managed deployments are unaffected. Bare / non-operator upgrades
either set DYN_SYSTEM_PORT or set DYN_SELF_HOST_METADATA=0.

All prerequisites are in main:
  #8855  worker HTTP hosting on system_status_server
  #9057  frontend MDC verify-and-cache pipeline
  #9610  harvest non-weight siblings into slug_dir
  #9707  worker advertises non-typed metadata siblings via extra_files
  #10037 native preprocessor consumes resolved local_dir
  #10599 register_model uses engine's runai-pulled local dir on
         object-storage URIs
  #10351 auto-clean MetadataArtifactRegistry on detach

Signed-off-by: nnshah1 <neelays@nvidia.com>
@nnshah1
nnshah1 force-pushed the nnshah1/gh-8749-default-on branch from dcc89a0 to 9568584 Compare July 8, 2026 23:36
The env-var parsing had no unit coverage after the default-on flip. Add tests
locking in the inverted semantics: unset -> on; the recognized falsy tokens
(0/false/no/off, any case, trimmed) opt out; explicit truthy plus empty or
unrecognized values keep the default-on behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Neelay Shah <neelays@nvidia.com>
@nnshah1
nnshah1 requested a review from a team as a code owner July 15, 2026 07:04
nnshah1 added 3 commits July 28, 2026 15:44
…lt-on

Signed-off-by: nnshah1 <neelays@nvidia.com>

# Conflicts:
#	lib/llm/src/local_model.rs
…t comment

Graham-review nits: OnceLock<()> -> Once (states run-once intent), and
drop the inline comment that duplicated the ENV_SELF_HOST_METADATA doc.

Signed-off-by: nnshah1 <neelays@nvidia.com>
parse_bool_opt already covers the falsy/truthy vocabulary in the truthy
crate; keep one assertion per bucket (unset/empty/unrecognized/falsy).

Signed-off-by: nnshah1 <neelays@nvidia.com>
@rmccorm4
rmccorm4 enabled auto-merge (squash) August 3, 2026 16:51
@rmccorm4
rmccorm4 merged commit a965876 into main Aug 3, 2026
113 checks passed
@rmccorm4
rmccorm4 deleted the nnshah1/gh-8749-default-on branch August 3, 2026 17:47
hhzhang16 added a commit that referenced this pull request Aug 4, 2026
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer

* 'main' of https://github.com/ai-dynamo/dynamo: (50 commits)
  docs(cli): correct removed vLLM prefill-worker flag reference (#12581)
  docs(operator): reserve webhook Ignore for emergencies (#12563)
  ci(docs): make previews and checks match what actually publishes (#12339)
  refactor(vllm): organize custom encoder modules (#12416)
  feat(llm): Select reasoning output field via env var (#11464)
  feat(runtime): add TLS support to TCP request plane (#10921)
  fix: convert conditional disagg sglang warning to httperror 400 (#12578)
  feat(operator): add runtime feature gates (#12421)
  refactor(runtime): extract PushRouter transport seam behind StreamingDispatch trait (#12447)
  feat(replay): add deterministic canonical offline reports (#12363)
  build: bump ModelExpress to 0.5.0(OPS-7978) (#12455)
  fix(mocker): use logical KV tokens for decode timing (#12583)
  fix(examples): update Triton example for CUDA 13 + fix libdcgm copy (DYN-3697) (#12577)
  refactor(operator): implement composition-first DGD reconciliation (#12283)
  feat(frontend): add basetenkenizer backend (#12376)
  fix(profiler): configure rapid mocker without planner (#12573)
  docs(vllm): correct worker-role flags and document --kv-transfer-config (#12568)
  ci: add Kubernetes deploy test to nightly (#12090)
  fix(container): reuse pinned protoc in runtime image (#12535)
  feat(self-host): flip DYN_SELF_HOST_METADATA default to ON (gh-8749) (#11417)
  ...

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
pvijayakrish pushed a commit that referenced this pull request Aug 11, 2026
…) (#12963)

Signed-off-by: Neelay Shah <neelays@nvidia.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Co-authored-by: Neelay Shah <neelays@nvidia.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actions deployment::k8s Relates to dynamo deployment in kubernetes documentation Improvements or additions to documentation feat frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants