Skip to content

fix: unbreak main — kv-router strict_priority call sites + vllm get_diff_sampling_param test mock - #10689

Merged
nnshah1 merged 3 commits into
mainfrom
neelays/fix-kv-router-strict-priority-callsite
Jun 13, 2026
Merged

nnshah1 merged 3 commits into
mainfrom
neelays/fix-kv-router-strict-priority-callsite

Conversation

@nnshah1

@nnshah1 nnshah1 commented Jun 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes a build break on main: dynamo-kv-router does not compile (E0061).

Cause

A semantic merge conflict between two PRs that merged the same day and do not overlap textually:

Git merged both cleanly (different files), but the result calls the 17-arg method with 16 args:

error[E0061]: this method takes 17 arguments but 16 arguments were supplied
  --> lib/kv-router/src/services/selection/core/mod.rs:429:14
note: method defined here
  --> lib/kv-router/src/scheduling/local.rs:254:18

This fails the build job on every open PR.

Fix

Pass strict_priority = 0 (the baseline tier) at both call sites. strict_priority is the high-order component of the scheduling sort key (strict_priority, policy_score); 0 is the default tier and preserves the pre-#10638 behavior of these callers (which had no concept of strict priority).

Verified locally with cargo check -p dynamo-kv-router (clean).

cc @PeaBrane (author of both #10638 and #10641) — flagging in case you would rather wire strict_priority through SelectRequest instead of defaulting; this PR is the minimal unbreak.

🤖 Generated with Claude Code


Open in Devin Review

Summary by CodeRabbit

  • Refactor
    • Adjusted internal priority handling during selection operations to improve request scheduling efficiency.

Related Issues

Standalone build fix — does not close a tracked issue. Resolves the dynamo-kv-router E0061 build break introduced on main by the #10638 + #10641 merge interaction.


Update: second independent main break folded in (commit d41d1bd)

While validating CI, a second broken-main failure surfaced — unrelated to kv-router and to this PR's original diff:

  • fix(vllm): reuse engine model config #10667 (fix(vllm): reuse engine model config) changed llm_engine.start() to read sampling params from vllm_config.model_config.get_diff_sampling_param().
  • fix(vllm): normalize unified served model name #10648 (fix(vllm): normalize unified served model name) added test_unified_start_returns_normalized_served_model_name with a mock that only puts get_diff_sampling_param on create_model_config() — not on vllm_config.model_config — so the test raises AttributeError on current main.

Same concurrent-merge pattern as the kv-router break. Because these are two independent breaks on main, no single-fix PR can reach green CI in isolation — so both unbreaks are bundled here so this PR can actually go green and unblock everyone. The vllm change is a 1-line test-mock fix (returns {}, matching the pre-#10667 path and the sibling test).

main does not compile: #10638 added a strict_priority: u32 parameter to
KvScheduler::schedule_with_block_hashes, but the runtime-free selection
service callers added concurrently in #10641 (select / select_and_reserve)
were not updated, so they call the method with 16 of 17 arguments (E0061).

The two PRs do not conflict textually (different files), so the merge was
clean but the result fails to build, breaking the build job on every open PR.

Pass strict_priority = 0 (baseline tier) at both call sites, preserving the
pre-#10638 behavior of these callers. Verified with cargo check -p dynamo-kv-router.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
@nnshah1
nnshah1 requested a review from a team June 13, 2026 04:38
@github-actions github-actions Bot added the fix label Jun 13, 2026

@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 1 potential issue.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Jun 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR adds a strict_priority parameter (set to 0 with a baseline tier designation) to the scheduler's schedule_with_block_hashes call in two methods: SelectionCore::select and SelectionCore::select_and_reserve. Both changes are identical and located in the selection core module.

Changes

Selection strict priority parameter

Layer / File(s) Summary
Add strict priority parameter to scheduler calls
lib/kv-router/src/services/selection/core/mod.rs
SelectionCore::select and SelectionCore::select_and_reserve both forward a new strict_priority parameter (value 0, baseline tier) to the scheduler's schedule_with_block_hashes method.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title is partially related to the changeset. It mentions 'kv-router strict_priority call sites' which accurately describes the change, but also includes vague language ('unbreak main') and references a second unrelated component ('vllm get_diff_sampling_param test mock') that does not appear in the file changes. Revise the title to focus on the primary change: 'fix(kv-router): pass strict_priority parameter in selection service call sites' or similar, removing vague terms and unrelated components.
✅ 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.
Description check ✅ Passed PR description is comprehensive and well-structured, clearly explaining the semantic merge conflict, root cause, and solution with adequate context.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Address review: SelectRequest / SelectAndReserveRequest exposed priority_jump
but not strict_priority, so selection-service callers were pinned to tier 0.

Add an optional strict_priority field (mirrors priority_jump: #[serde(default)]
Option), and pass req.strict_priority.unwrap_or(0) to the scheduler instead of
a hard-coded 0. Backward compatible — omitted field defaults to baseline tier 0.
Matches how push_router/selection.rs already sources strict_priority from
routing metadata. Verified with cargo check -p dynamo-kv-router.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
@nnshah1
nnshah1 enabled auto-merge (squash) June 13, 2026 08:56
Second main build/test break (independent of the kv-router one in this PR):
#10667 changed llm_engine.start() to read sampling params from
vllm_config.model_config.get_diff_sampling_param(), but the test added
concurrently in #10648 (test_unified_start_returns_normalized_served_model_name)
mocks vllm_config.model_config as SimpleNamespace(max_model_len=4096) — without
that method — so the test raises AttributeError on current main.

Add get_diff_sampling_param to the model_config mock (returns {}, matching the
pre-#10667 create_model_config path and the sibling test at L526). Pure test
mock fix; no production change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
@nnshah1
nnshah1 requested review from a team as code owners June 13, 2026 17:21
@github-actions github-actions Bot added the backend::vllm Relates to the vllm backend label Jun 13, 2026
@nnshah1 nnshah1 changed the title fix(kv-router): unbreak main — pass strict_priority in selection service call sites fix: unbreak main — kv-router strict_priority call sites + vllm get_diff_sampling_param test mock Jun 13, 2026
@nnshah1

nnshah1 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up @PeaBrane: folded a second, independent main-unbreak into this PR (commit d41d1bd3bd) — a 1-line vLLM test-mock fix for get_diff_sampling_param (broken by #10667 ↔ #10648, same concurrent-merge pattern as the kv-router issue). Bundled because main currently has two independent breaks and neither fix can reach green CI on its own. Flagging since it widens scope beyond the kv-router change you approved.

@nnshah1
nnshah1 merged commit 8ddba58 into main Jun 13, 2026
100 checks passed
@nnshah1
nnshah1 deleted the neelays/fix-kv-router-strict-priority-callsite branch June 13, 2026 18:10
yao531441 pushed a commit to yao531441/dynamo that referenced this pull request Jun 24, 2026
…ice call sites (ai-dynamo#10689)

Signed-off-by: nnshah1 <neelays@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

This branch was previously deployed

1 inactive deployment
GITLAB — d41d1bd3 Deployed Jun 13, 2026 by copy-pr-bot[bot] via Trigger CI Pipeline #31205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::vllm Relates to the vllm backend fix size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants