Skip to content

[Bug] Skip ONNX weights when downloading candle embedding models - #3533

Open
vuhluu wants to merge 2 commits into
vllm-project:mainfrom
vuhluu:fix/modeldownload-candle-skip-onnx-weights
Open

[Bug] Skip ONNX weights when downloading candle embedding models#3533
vuhluu wants to merge 2 commits into
vllm-project:mainfrom
vuhluu:fix/modeldownload-candle-skip-onnx-weights

Conversation

@vuhluu

@vuhluu vuhluu commented Sep 6, 2026

Copy link
Copy Markdown

Closes #3532

Purpose

The runtime model downloader (src/semantic-router/pkg/modeldownload) ran hf download <repo> --local-dir <path> with no file filter, so a candle deployment of llm-semantic-router/mmbert-embed-32k-2d-matryoshka fetched the whole repository: about 4.3 GB of onnx/**/model.onnx, model.onnx.data and model_fa_fp16.onnx exports on top of the ~650 MB (config.json, model.safetensors, tokenizer.json) the candle runtime actually loads. On a fresh machine that blocks router startup for the whole transfer and can exhaust disk before the embedding model is usable.

This change:

  • adds ModelSpec.ExcludePatterns, forwarded to hf download by a new pure helper buildDownloadArgs as one --exclude <pattern> flag per pattern. The typer-based hf CLI takes --exclude as a repeatable single-value option (one flag followed by several patterns keeps only the first and treats the rest as positional filenames), while the legacy huggingface-cli took nargs=*; the repeated form is parsed identically by both, whichever CLI the image ends up installing;
  • populates it with *.onnx, *.onnx.data, *.onnx_data for every candle embedding model path (mmBERT, Qwen3, Gemma, multimodal) when EmbeddingModels.EmbeddingBackend() is candle. The exclude map is keyed and looked up by config.ResolveModelPath, so the narrowing holds when the model is configured by a registry alias and does not depend on whether the collected provisioning paths are canonicalized first ([Router] Download aliased model paths to the directory the runtime loads #2828 can land before or after this);
  • leaves OpenVINO deployments on the full snapshot (they consume the ONNX exports), leaves remote backends unchanged (they provision nothing), and leaves non-embedding models (classifiers, semantic-cache BERT) unchanged to keep the blast radius on the embedding runtime;
  • excludes weight files only, so the onnx/model_config.json layer manifest read by config.MmBertAvailableLayers is still downloaded.

Modules affected: src/semantic-router/pkg/modeldownload only (router service platform surface, router-core rules). Owner: wg/router-models-inference-runtime (accepted issue #3532).

Test Plan

  • make agent-report ENV=cpu CHANGED_FILES="src/semantic-router/pkg/modeldownload/downloader.go,src/semantic-router/pkg/modeldownload/config_parser.go,src/semantic-router/pkg/modeldownload/types.go,src/semantic-router/pkg/modeldownload/downloader_test.go,src/semantic-router/pkg/modeldownload/download_scope_test.go" → primary skill project-change, fast tests make test-semantic-router
  • Unit tests in downloader_test.go and download_scope_test.go:
    • TestBuildDownloadArgsFetchesFullSnapshotByDefault: models without a scope keep the historical argument list
    • TestBuildDownloadArgsRepeatsExcludeFlagPerPattern: every pattern carries its own --exclude, the flag count equals the pattern count, --exclude trails --revision
    • TestBuildDownloadArgsSkipsEmptyExcludePatterns: an empty entry never produces a bare --exclude
    • TestBuildModelSpecsExcludesOnnxWeightsForCandleEmbeddingModels: all four candle paths get the exclude list
    • TestBuildModelSpecsExcludesOnnxWeightsForAliasedEmbeddingModel: models/mom-embedding-ultra (alias) and the canonical path are both narrowed; matches specs by resolved LocalPath, so it passes with or without [Router] Download aliased model paths to the directory the runtime loads #2828
    • TestBuildModelSpecsKeepsFullSnapshotForOpenVINOBackend: OpenVINO is unfiltered
    • TestBuildModelSpecsLeavesNonEmbeddingModelsUnfiltered: semantic-cache BERT is unfiltered
    • TestOnnxWeightExcludePatternsNeverMatchCandleRequiredFiles: exclude globs can never shadow a completeness-required file (would otherwise cause an endless re-download loop)
  • cd src/semantic-router && go test ./pkg/modeldownload/... && go vet ./pkg/modeldownload/...
  • gofmt -l src/semantic-router/pkg/modeldownload/
  • Exclude globs checked against the live repository file list with huggingface_hub.utils.filter_repo_objects (the filter hf download --exclude uses)
  • End-to-end download check: in a clean python:3.12-slim container with unpinned huggingface_hub (the same install as tools/docker/Dockerfile.extproc and test-and-build.yml), run the exact argv buildDownloadArgs produced before and after the fix against the live llm-semantic-router/mmbert-embed-32k-2d-matryoshka repository into a fresh --local-dir, then list the files on disk
  • make agent-ci-gate CHANGED_FILES="..." (same file list)

Test Result

  • go test ./pkg/modeldownload/... -v -run 'DownloadArgs|ExcludesOnnx|OpenVINO|Unfiltered|NeverMatch|Aliased': 8/8 PASS (macOS, Intel, Go 1.25.0)
  • go test ./pkg/modeldownload/...: ok … 1.323s; go vet ./pkg/modeldownload/...: clean; gofmt -l: no output
  • Exclude globs vs. the repository file list: 16 files kept (including config.json, model.safetensors, tokenizer.json, onnx/model_config.json and the per-layer onnx/*/config.json), 12 dropped (every model.onnx, model.onnx.data, model_fa_fp16.onnx)
  • End-to-end download check, hf 1.30.0:
    • before (--exclude "*.onnx" "*.onnx.data" "*.onnx_data", the argv of the first revision): UserWarning: Ignoring '--exclude' since filenames have been explicitly set; Fetching 4 files, 2.12 GB: the directory holds only onnx/layer-{6,11,16,22}/model.onnx.data; no model.safetensors, no tokenizer.json. Reproduces the exact-head CI failure: the stray positionals are treated as filename globs, so the CLI downloads precisely the ONNX weights and nothing the candle runtime needs
    • after (--exclude "*.onnx" --exclude "*.onnx.data" --exclude "*.onnx_data"): Fetching 16 files, 614 MB; model.safetensors, tokenizer.json, config.json, onnx/model_config.json and the per-layer onnx/*/config.json present; find for *.onnx, *.onnx.data, *.onnx_data returns nothing
  • make agent-ci-gate: passed on the first revision (fd526d02: agent-report, pre-commit baseline, Go structural lint, structure and architecture checks); not re-run on 86ce721d, which only touches the same package's Go sources and tests
  • Manual fresh download through the router binary itself: not re-run locally (the checkout already holds the minimal model directory, so the downloader treats it as complete)

Follow-ups tracked outside this PR: classifier repositories also ship onnx/ subtrees and are intentionally not narrowed here because the ONNX classifier backend may need them (follow-up on #3532); IsGatedModelError returning true for any failure when HF_TOKEN is empty, which turns a transient public-repo failure into a "gated, skipping" startup (raised by @adaamko in review, to be filed separately).


Semantic Router PR Checklist
  • PR title begins with exactly one bracketed category, such as [Feature], [Bug], [Docs], [Test], [Research], [Community], or [CI/Build]
  • The title does not stack prefixes such as [Router][Docs]; affected modules belong in labels and the PR body
  • The PR links an accepted issue with exactly one owner: one wg/* label for project work or owner/maintainers for repository governance
  • Commits in this PR are signed off with git commit -s
  • The Purpose, Test Plan, and Test Result sections reflect the actual scope, commands, and blockers for this change

@github-actions github-actions Bot added pr/needs-rebase Needs rebase or conflict resolution. wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup. labels Sep 6, 2026
@netlify

netlify Bot commented Sep 6, 2026

Copy link
Copy Markdown

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 86ce721
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/6a9e969a60a5b10007f915df
😎 Deploy Preview https://deploy-preview-3533--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

The runtime downloader invoked `hf download <repo> --local-dir <path>` for
every missing model, so a candle deployment of
llm-semantic-router/mmbert-embed-32k-2d-matryoshka pulled the whole
repository: roughly 4.3 GB of onnx/**/model.onnx and model.onnx.data
exports on top of the ~620 MB of config.json, model.safetensors and
tokenizer.json that the candle runtime actually loads. On a local machine
this blocked router startup for the duration of the transfer and could
exhaust disk before the embedding model was usable.

Give ModelSpec an ExcludePatterns field that is forwarded to
`hf download --exclude`, and populate it for the candle embedding model
paths (mmbert, qwen3, gemma, multimodal) when the embedding backend is
candle. OpenVINO keeps the full snapshot because it consumes the ONNX
exports, remote backends provision nothing, and other local models keep
their current behaviour. Only weight files are excluded, so the
onnx/model_config.json layer manifest read by MmBertAvailableLayers is
still fetched, and a test guards that no exclude pattern can ever match a
completeness-required file.

Signed-off-by: Vu Luu <luuhavu@gmail.com>
@vuhluu
vuhluu force-pushed the fix/modeldownload-candle-skip-onnx-weights branch from 1d22597 to fd526d0 Compare September 6, 2026 17:31
@github-actions github-actions Bot added pr/blocked Blocked on a named decision, dependency, or required check. and removed pr/needs-rebase Needs rebase or conflict resolution. labels Sep 6, 2026

@Xunzhuo Xunzhuo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Welcome, and thanks for the focused download-scope fix. One blocker: hf download --exclude accepts one pattern per option, but buildDownloadArgs emits one flag followed by all three patterns, so the remaining patterns become positional filenames. Exact-head CI reports the exclusion ignored and leaves model.safetensors absent. Please repeat --exclude for each pattern, update the argv test, and rerun the required checks.

@adaamko adaamko 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.

agree with the fix and with xunzhuo's blocker. one reason it bit: the image installs huggingface_hub[cli] unpinned, and the new hf download (typer) takes --exclude as a repeatable option, while the old huggingface-cli download took nargs=*. repeating the flag works on both, so thats the safe form whichever cli the image ends up with.

two more from #3498, whose E2E this download broke twice. IsGatedModelError returns true for any failure when HF_TOKEN is empty (the noToken branch), so a 429 or a connection reset on this exact repo gets logged as "gated model, skipping" and the router comes up with embedding_ready:false; semantic-cache then fails 0/15 and nothing else notices. this PR makes the fetch 7x smaller, which helps, but the skip branch is the real bug: a public repo that fails to download should retry and then fail startup, not get skipped as gated. follow-up, not this PR, i can file it.

and a heads up: #2828 canonicalizes the embedding paths through ResolveModelPath in the same function, and your exclude map is keyed by the raw cfg path. whichever of the two lands second has to key on the resolved path or the exclusion silently stops applying for aliased configs.

The typer-based `hf download` takes `--exclude` as a repeatable
single-value option, so `--exclude *.onnx *.onnx.data *.onnx_data` kept
only the first pattern and passed the other two as positional filenames.
hf 1.30.0 then warns "Ignoring `--exclude` since filenames have been
explicitly set", treats the stray positionals as filename globs, and
fetches exactly the onnx/*/model.onnx.data files (4 files, 2.12 GB)
while model.safetensors is never downloaded. This is the failure
exact-head CI reported on this PR.

buildDownloadArgs now emits `--exclude <pattern>` once per pattern
(skipping empty entries), the form the typer `hf` CLI and the legacy
nargs=* `huggingface-cli` parse identically.

Also key the candle exclude map on config.ResolveModelPath on both the
build and lookup side, so the narrowing still applies when the embedding
model is configured by a registry alias and regardless of whether the
collected provisioning paths are canonicalized first (vllm-project#2828).

Tests: TestBuildDownloadArgsRepeatsExcludeFlagPerPattern replaces the
argv test and asserts one flag per pattern; new
TestBuildDownloadArgsSkipsEmptyExcludePatterns and
TestBuildModelSpecsExcludesOnnxWeightsForAliasedEmbeddingModel.

Verified against llm-semantic-router/mmbert-embed-32k-2d-matryoshka in a
clean python:3.12-slim container with unpinned huggingface_hub (hf
1.30.0): 16 files / 614 MB; model.safetensors, tokenizer.json and
onnx/model_config.json present; no *.onnx, *.onnx.data or *.onnx_data.

Signed-off-by: Vu Luu <luuhavu@gmail.com>
@vuhluu

vuhluu commented Sep 7, 2026

Copy link
Copy Markdown
Author

@Xunzhuo thanks, fixed in 86ce721: one --exclude flag per pattern, argv test updated (TestBuildDownloadArgsRepeatsExcludeFlagPerPattern). Reproduced outside CI with hf 1.30.0: the old argv fetched only onnx/*/model.onnx.data (4 files, 2.12 GB, no model.safetensors); the new argv fetches 16 files / 614 MB with model.safetensors and no ONNX weights. Details in the Test Result section.

@adaamko thanks, went with the repeated flag for exactly that reason. For #2828, the exclude map is now keyed by config.ResolveModelPath on both sides (TestBuildModelSpecsExcludesOnnxWeightsForAliasedEmbeddingModel), so it holds whichever lands second. Agree on the IsGatedModelError skip branch: please file it, happy to take it as a follow-up PR.

@github-actions github-actions Bot added pr/needs-author Waiting for author changes or response. and removed pr/blocked Blocked on a named decision, dependency, or required check. labels Sep 7, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 34.17%. Comparing base (aff932e) to head (86ce721).
⚠️ Report is 235 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3533      +/-   ##
==========================================
+ Coverage   33.93%   34.17%   +0.24%     
==========================================
  Files          20       20              
  Lines        2959     2888      -71     
==========================================
- Hits         1004      987      -17     
+ Misses       1849     1795      -54     
  Partials      106      106              
Flag Coverage Δ
operator 34.17% <ø> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/needs-author Waiting for author changes or response. wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Runtime model downloader fetches multi-GB ONNX exports the candle embedding runtime never loads

4 participants