[Router] Require classifier runtime weights so partial downloads heal - #2820
[Router] Require classifier runtime weights so partial downloads heal#2820olifarhaan wants to merge 2 commits into
Conversation
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned when their GitHub accounts are assignable in this repository: 📁
|
✅ Supply Chain Security Report — All Clear
Scanned at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2820 +/- ##
==========================================
+ Coverage 33.83% 33.93% +0.09%
==========================================
Files 20 20
Lines 2935 2959 +24
==========================================
+ Hits 993 1004 +11
- Misses 1840 1849 +9
- Partials 102 106 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
On the mmBERT-32K backend the category, PII, and jailbreak classifiers load through TraditionalModernBertTokenClassifier, which hard-reads config.json, tokenizer.json, and model.safetensors from the model root. Completeness only demanded config.json plus any weight-shaped file anywhere in the tree, so an interrupted download that left a companion mapping and a nested adapter blob behind read as complete and was never re-fetched, while init kept failing. Mirror addEmbeddingModelRequiredFiles and record the root weights and tokenizer for these three models. Restrict it to the mmBERT-32K path, which initialises with no fallback; the candle variant auto-detects LoRA directories that legitimately carry adapter weights instead, and requiring root weights there would strand a valid model in a permanent re-download loop. A prompt guard with a Protocol set runs remotely and has no local model at all. Signed-off-by: Mohammad Ali Farhan <alifarhan231087@gmail.com>
Pin the required-file contract for all three classifiers, the partial-download directory that used to read as complete, the fully downloaded control, and the candle backend that must keep the looser heuristic. Signed-off-by: Mohammad Ali Farhan <alifarhan231087@gmail.com>
0ef8833 to
75387de
Compare
|
Thanks for adding the classifier completeness regressions. This branch currently conflicts with main, so please rebase it onto current main and rerun the model-download tests; I will review the refreshed head. |
Xunzhuo
left a comment
There was a problem hiding this comment.
Provisioning discovers request-reachable named recipes, but required classifier files are added only from flat/default fields. A named reachable mmBERT recipe can therefore download without mandatory root weights/tokenizer and be accepted as complete. Please apply completeness requirements to the same reachable profiles and cover a named entrypoint.
adaamko
left a comment
There was a problem hiding this comment.
agree with xunzhuo on named recipes, and the fix is probably structural rather than another loop: ExtractRequiredFilesByModel already walks the config by reflection and picks up mapping files from named recipes for free, while addClassifierModelRequiredFiles reads three flat fields. if the root-weight rule hangs off the same walk (any CategoryModel / PIIModel / PromptGuardConfig it visits, with the same mmbert32k + local checks), reachable named recipes are covered without a second list to maintain.
on the carve-out: the comment and test say "LoRA", but what the code skips is the whole candle backend, and a merged non-LoRA directory on candle is exactly as exposed to the half-download case as the mmbert32k one. fine to leave that for later, but name it as the candle backend so nobody reads the LoRA test as covering it.
conflicts with #3533 and #2828 in the same function, see my note on #2828 for an order.
Closes #2669
Purpose
On the mmBERT-32K backend, which is the default for all three, the category, PII, and jailbreak classifiers load through
TraditionalModernBertTokenClassifier::new_with_variant. It hard-readsconfig.json,tokenizer.json, andmodel.safetensorsfrom the model root and fails outright if any is missingCompleteness only demanded
config.jsonplus any weight-shaped file, andhasModelWeightsmatches*.safetensors,*.bin, and*.onnxrecursively. So an interrupted download that left the companion mapping and a nested adapter blob behind read as complete, was never re-fetched, and since these classifiers register withbestEffort: false, every restart failed the same wayaddClassifierModelRequiredFilesmirrors the existingaddEmbeddingModelRequiredFilesfrom #2172 and records the root weights and tokenizer for the three models. Module affected:RouterOn the LoRA caveat raised in the issue: this is gated on
use_mmbert_32k, which is exactly the flag selecting the initialiser that has no LoRA fallback. With it off, PII and jailbreak init auto-detects LoRA directories that legitimately carry adapter weights instead of root weights, so those keep the looser heuristic. A test pins that carve-outTest Plan
To confirm the tests are not vacuous, revert each part of the fix and re-run
Test Result
All four new tests fail on the base for the right reason, for example:
Reverting each part of the change in turn fails at least one test every time:
addClassifierModelRequiredFilescall droppedTestBuildModelSpecsRequiresClassifierRuntimeWeights,TestPartialClassifierDirReportedIncompleteuse_mmbert_32kgate dropped, so LoRA also demands root weightsTestLoRAClassifierKeepsHeuristicCompletenessmake go-lintreports 0 issues,make check-go-mod-tidyandmake agent-ci-lintpass, and every applicable pre-commit hook passes.make test-semantic-routerreports one failure,TestHybridCachePendingRequestinpkg/cache, which is a known flake unrelated to this change. It fails on other contributors' PRs that do not touchpkg/cacheeither (#2530, #2507, #2810), and it passed on my own PR #2819. The test writes to the Milvus-backed hybrid cache, sleeps a fixed 100ms "for indexing", then asserts the entry is findable, so a loaded runner fails it. Locally it cannot run at all without a Milvus instance, which is whymake test-semantic-routerdefaultsSKIP_MILVUS_TESTS=true. It is currently failing on several unrelated PRs and onmainitselfOne thing I did not change:
DefaultRequiredFilesstill allows the loose nested-weight heuristic for every other model. Tightening that globally would be a much wider change than this issueThe
models/prefix is now a named constant, since the linter flags the third repetition this change would have introduced