Repository navigation
[Bug] Keep decision adaptations when the Builder deploys - #4535
Conversation
The Builder decompiles the config to DSL, compiles it back, and deploys the compiled routing in replace mode. The DSL cannot express decision adaptations, and mergeDSLOwnedNodes replaced routing and recipes with the compiled nodes, so a Deploy without edits removed every decision's adaptations: 4 decisions in the default config, 2 in the agent recipe and all 16 in the built-in mom-v1 recipe. Those decisions then fall back to mode apply, and Router Learning acts on routes that were set to bypass or observe it. Before the DSL-owned sections are replaced or merged, copy each base decision's adaptations into the compiled decision with the same name, matching recipes by name first. A value already in the compiled fragment wins. This is the rule dsl.MergeRoutingIntoBase applies when sr-dsl compiles with --base. Signed-off-by: Wilson Wu <iwilsonwu@gmail.com>
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for preserving decision adaptations across Builder deploys. The new round-trip test includes an archived v0.4 config that the current parser rejects before checking adaptations; please use a compatible fixture and rerun the Dashboard suite.
Since vllm-project#4734 removed routing.data_policy from the schema, the current parser rejects the immutable v0.4 mom-v1 snapshot before the test reaches any adaptations check. Round-trip only the maintained configs: config/config.yaml, the agent recipe and the latest built-in mom-v1, which carries the same 16 decision adaptations as the snapshot. Signed-off-by: Wilson Wu <iwilsonwu@gmail.com>
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for updating the fixture. The maintained-config round trips and Dashboard suite now pass, including preserving decision adaptations across both deploy modes.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status🛑 Queue command has been cancelled |
Merge Queue Status🛑 Queue command has been cancelled |
Merge Queue Status🛑 Queue command has been cancelled |
Merge Queue Status
This pull request spent 3 minutes 37 seconds in the queue, with no time running CI. ReasonPull request #4535 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
Closes #4534
Purpose
Deploying from the Builder removed
adaptationsfrom every decision, even without edits. The Builder deploys DSL-compiled routing in replace mode. The DSL cannot expressadaptations, andmergeDSLOwnedNodesreplacedroutingandrecipeswith the compiled nodes. Those decisions then fell back tomode: apply, so Router Learning started to act on routes that were set to bypass or observe it.This change touches only the dashboard backend:
preserveBaseDecisionAdaptations(deploy_policy_preservation.go):adaptationsinto the compiled decision with the same name.mergeDSLOwnedNodescalls it in both deploy modes. Merge mode also replaces a supplieddecisionsorrecipeslist as a whole, so DSL-compiled fragments lostadaptationsthere too.dsl.MergeRoutingIntoBasealready applies forsr-dsl compile --base.Owner: Developer Experience & Ecosystem Workgroup (dashboard).
Test Plan
go test ./handlers -run 'TestMergeDeployPayload|TestDeploy'indashboard/backend. New tests:TestMergeDeployPayloadKeepsBaseDecisionAdaptations, in replace and merge mode:TestMergeDeployPayloadReplaceKeepsMaintainedDecisionAdaptations:config/config.yaml, the agent recipe, and both built-in mom-v1 configsmake dashboard-checkmake check CHANGED_FILES="<the 3 changed files>"config.yaml.Test Result
The new tests fail on main (all 4 maintained configs, and both modes of the unit test) and pass here. All deploy handler tests pass.
make dashboard-checkandmake checkpass.Manual, main → this fix:
adaptationsadaptationsin the written configThe remaining preview changes are other DSL round-trip differences, such as list order and fields the DSL does not carry. This PR does not change them.
With the default config, Deploy Now fails validation on main and here for a separate reason. The DSL round trip adds the
static_business_routecandidate iteration model tomodelRefs, and its LoRAbusiness-adapteris not declared inmodelCards. A config merged bysr-dsl compile --basefails the same validation.A renamed decision does not keep the old name's adaptations, as with
sr-dsl compile --base.I did not run the optional
dashboardE2E profile locally because it needs a kind cluster. CI selects it.Before fix:

After fix:

Semantic Router PR Checklist
[Feature],[Bug],[Docs],[Test],[Research],[Community], or[CI/Build][Router][Docs]; affected modules belong in labels and the PR bodyacceptedissue with exactly one owner: onewg/*label for project work orowner/maintainersfor repository governancegit commit -sSee CONTRIBUTING.md for the full contributor workflow and commit guidance.