[Test] Migrate the dashboard profile values to canonical v0.3 - #3505
[Test] Migrate the dashboard profile values to canonical v0.3#3505rishabhsinha17 wants to merge 3 commits into
Conversation
The dashboard profile values still used the pre-v0.3 schema. The loader dropped every legacy key, so the router ran on chart-default placeholders while the file claimed otherwise. Rewrite the config block in canonical v0.3, encoding what the legacy keys declared: one base-model with a general-expert LoRA, one "other" domain, one catch-all decision, priority strategy, response cache disabled. The fixture now declares routing.modelCards, so the safe-failure deploy fragment no longer needs to carry its own model surface; drop the per-test workaround and register the file in the maintained-asset contract so it cannot regress to a legacy shape. Part of vllm-project#3233 Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu>
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for migrating the shared fixture. The requested config/read/deploy follow-through is incomplete: dashboard-config-read still only checks a nonempty JSON object and nonempty YAML body. Both assertions pass if the Dashboard continues serving chart placeholders instead of the new base-model/other_decision fixture, so this prerequisite still does not prove the user-facing read surfaces reflect the migrated config. Please assert canonical identifying fields on both JSON and YAML reads while retaining the deploy safe-failure assertion.
…lues-v03 Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu> # Conflicts: # e2e/profiles/dashboard/values.yaml
dashboard-config-read only checked for a nonempty JSON object and a nonempty YAML body, which also passes when the Dashboard serves chart placeholders instead of the deployed fixture. Assert the canonical identifying fields on both read paths: providers.models must declare base-model and routing.decisions must declare other_decision, matching e2e/profiles/dashboard/values.yaml. Both responses decode into JSON-shaped maps, so one walker covers the JSON and the YAML endpoint. Part of vllm-project#3233 Signed-off-by: Rishabh Sinha <rsinha17@terpmail.umd.edu>
|
Reworked per the review. dashboard-config-read now asserts the canonical identifying fields on both read paths: config/all and config/yaml must each declare base-model under providers.models and other_decision under routing.decisions, checked by one walker over the JSON-shaped decode of each response. A Dashboard serving chart placeholders now fails both reads instead of passing on nonempty output. The deploy safe-failure assertion is retained unchanged. The branch also merges current main to pick up the model catalog schema change from #3489, respelling the fixture to the new canonical fields (defaults.model, reasoning.family, backend_refs provider). The 09-05 dashboard go mod tidy failure does not reproduce after the merge. Verified locally: the maintained-asset contract suite over the migrated file, dashboard frontend tests and type-check, both go mod tidy checks, and the changed-file pre-commit hooks. |
|
@Xunzhuo checks are green on the rework, ready for another look. Review re-request via gh needs triage permission on this repo, so pinging here instead. |
Related #3233
Standalone prerequisite requested in #3233 (comment): migrate
e2e/profiles/dashboard/values.yamloff the pre-v0.3 schema before the next dashboard journey lands, so tests stop accumulating per-testmodelCardsworkarounds around a stale shared fixture.Legacy vs effective
The old file declared
model_config/categories/decisions/strategy/default_model/bert_model/classifier/prompt_guard/response_cacheat the top level of the config block. The v0.3 loader drops every one of those keys, so what the router actually ran was the chart defaults:replace-with-your-modelplaceholder providers and modelCards, thegeneraldomain, thedefault-routedecision, plus the file's one surviving key (global.model_catalog.kbs: []). The file documented a router that never existed.What changed
e2e/profiles/dashboard/values.yamlis rewritten in canonical v0.3 (version/listeners/providers/routing/global, mirroringe2e/profiles/ai-gateway/values.yaml), encoding what the legacy keys meant where that is discernible:model_config."base-model"(qwen3, LoRAgeneral-expert) ->providers.models[base-model]+routing.modelCardswith the same LoRA and description. The backend ref points atvllm-llama3-8b-instruct.default.svc.cluster.local:8000, which this profile already deploys through the shared gateway stack resource manifests.categories: [other]->routing.signals.domains: [other].decisions(other_decision) ->routing.decisions, unchanged in content.strategy: priority->global.router.strategy: priority.default_model: general-expert->providers.defaults.model(the post-[Feature] Add unified model catalog and model hub #3489 canonical spelling; the merge with current main also respellsreasoning.familyand addsprovidertobackend_refs), with a baregeneral-expertmodelCard so the reference resolves, same pattern as the ai-gateway profile.response_cache.enabled: false->global.stores.response_cache.enabled: false. This is an intent-encoding change of effective behavior: the dropped legacy key meant disabled, while the chart default had it enabled.prompt_guard.enabled: false-> no jailbreak signal declared, which is how v0.3 spells disabled.bert_model/classifiermodel IDs and thresholds are dropped rather than translated: they have no canonical top-level home, they duplicated loader/registry defaults, and the effective config never contained them anyway.listeners: []follows the convention of every migrated e2e profile on this gateway stack. This drops the chart-default router-owned listeners from the rendered config; the ExtProc/API servers are not configured throughlisteners, which twenty-odd profiles on the same stack already demonstrate.Follow-through edits keep the assertions honest:
e2e/testcases/dashboard_deploy_safe_failure.go: the fragment no longer carries its ownrouting.modelCards. The fixture now declares the model surface, merge mode leaves that node untouched, so the unknown-model check fires against the shared fixture instead of a per-test workaround. Verified locally: the merged document is rejected withrouting.decisions[e2e-safe-failure].modelRefs[e2e-nonexistent-model] references unknown model "e2e-nonexistent-model".e2e/testcases/dashboard_config_read.go(review follow-up):config/allandconfig/yamlmust each declarebase-modelunderproviders.modelsandother_decisionunderrouting.decisions, decoded into JSON-shaped maps and checked by one walker, instead of passing on any nonempty payload. A Dashboard serving chart placeholders now fails both reads.src/semantic-router/pkg/config/maintained_asset_contract_test.go: the file is registered inmaintainedValuesConfigAssets, soTestMaintainedConfigAssetsUseCanonicalV03Contractnow parses it with the real loader and rejects any regression to legacy keys.Verification
go test ./pkg/config/ -run TestMaintainedConfigAssetsUseCanonicalV03Contractpasses with the new asset registered.helm templateoverdeploy/helm/semantic-routerwith the new values renders a config with no legacy keys; the rendered document parses viaParseYAMLBytes, and the safe-failure merge is rejected by the unknown-model validator as described above.go build ./cmd/e2e,go vet, yamllint clean on the touched files.selection: prin the domain registry fore2e/profiles/dashboard/**ande2e/testcases/dashboard_*.go) is the real proof and triggers on this PR.