Skip to content

refactor(mm-routing): propagate image token accounting failures - #16947

Merged
krishung5 merged 1 commit into
ai-dynamo:mainfrom
HanFa:codex/dis-3123-image-token-accounting-errors
Oct 21, 2026
Merged

krishung5 merged 1 commit into
ai-dynamo:mainfrom
HanFa:codex/dis-3123-image-token-accounting-errors

Conversation

@HanFa

@HanFa HanFa commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Make per-image token accounting fallible so routing planners can reject unsupported geometry without substituting a zero or approximate count. If any image fails accounting, disable exact multimodal routing and image-token metrics for the request while preserving the worker-bound prompt and media.

Details

  • Propagate accounting errors through the image backend interface and default batch implementation.
  • Handle failures in decoded-image, URL-dimension, and mixed-media preprocessing paths, using the existing text-prefix routing fallback and routing/hash installation gate.
  • Preserve existing registry and Nemotron calculations by wrapping successful counts in Ok.
  • Add backend-independent tests covering successful requests and failures before or after a valid image count; verify that partial totals and routing hashes are not installed.

Stack

This is the prerequisite for the GLM-5.3-Flash image planner integration. It introduces no GLM dependency or model-specific layout hook. The follow-up is HanFa#3, based on this PR's head branch in the contributor fork so it shows only the GLM integration diff. After this prerequisite merges, the GLM commit will be submitted in a separate upstream PR.

Where should the reviewer start?

Start with ImageRoutingBackend in lib/llm/src/preprocessor/mm_routing/image.rs, then review the three accounting call sites in lib/llm/src/preprocessor.rs.

Validation

  • cargo test -p dynamo-llm --no-default-features --features mm-routing --locked --lib preprocessor:: -- --test-threads=4: 376 passed, 2 ignored on this commit alone.
  • cargo fmt --all --check and git diff --check: passed.
  • Repository pre-commit and commit-message hooks, including DCO: passed.
  • Validated locally on macOS; backend GPU E2E is outside this prerequisite change.

Related Issues

Part of #16948.

Internal tracking: DIS-3123 (multimodal-aware KV routing model support).

Return Result from image token accounting and propagate failures through batch counting and preprocessing. Disable exact routing and image-token metrics when accounting fails, while preserving worker-bound inputs.

Add backend-independent tests for failures before and after a valid image count, including atomic routing/hash fallback.

Validation: targeted preprocessor Rust tests passed (376 passed, 2 ignored); cargo fmt and git diff --check passed.
Signed-off-by: Fang Han <fangh@nvidia.com>
@HanFa
HanFa requested review from a team as code owners October 20, 2026 21:29
@copy-pr-bot

copy-pr-bot Bot commented Oct 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@HanFa
HanFa temporarily deployed to external_collaborator October 20, 2026 21:29 — with GitHub Actions Inactive
@HanFa
HanFa temporarily deployed to external_collaborator October 20, 2026 21:29 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi HanFa! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added the external-contribution Pull request is from an external contributor label Oct 20, 2026
@github-actions github-actions Bot added the frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` label Oct 20, 2026
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 63b1cadf-eb70-4f46-8197-33a92f103a2d
📥 Commits

Reviewing files that changed from the base of the PR and between 384b5c4 and 1728307.

📒 Files selected for processing: 4
  • lib/llm/examples/mm_token_count.rs
  • lib/llm/src/preprocessor.rs
  • lib/llm/src/preprocessor/mm_routing/image.rs
  • lib/llm/src/preprocessor/mm_routing/nemotron.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Image token counting now returns errors through the routing API. Preprocessing handles failures by clearing image-token metrics and disabling or skipping exact MM routing while continuing request preprocessing.

Changes

MM image token counting

Layer / File(s) Summary
Fallible image-counting API
lib/llm/src/preprocessor/mm_routing/image.rs, lib/llm/src/preprocessor/mm_routing/nemotron.rs, lib/llm/examples/mm_token_count.rs
The routing backend and processor return Result<usize>. The Nemotron implementation wraps its existing count in Ok, and the example propagates counting errors.
Preprocessing behavior on count failures
lib/llm/src/preprocessor.rs, lib/llm/src/preprocessor/mm_routing/image.rs
When image counting fails, preprocessing clears image-token metrics and disables exact MM routing. Exact routing expansion logs a warning and skips MM routing info. Tests cover error propagation and preservation of request data.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 17283

Image-counting failures leave request data available while disabling exact multimodal routing and image-token metrics. No identified issue prevents merging after normal checks.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: propagating image token accounting failures in MM routing.
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 The description covers the change overview, implementation details, reviewer starting point, validation, and related tracking. The required Related Issues section references issue #16948, although it …
Full details: Description check

Explanation

The description covers the change overview, implementation details, reviewer starting point, validation, and related tracking. The required Related Issues section references issue #16948, although it uses a full URL and “Part of” instead of the template’s preferred “Closes” or “Relates to” syntax.

Full details: Docstring Coverage

Explanation

Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files. (1 skipped: 1 too large.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@HanFa

HanFa commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

/ok-to-test 1728307

@HanFa

HanFa commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 1728307

@jthomson04

Copy link
Copy Markdown
Contributor

/ok to test 1728307

@devin-ai-integration

Copy link
Copy Markdown
Contributor

⏹️ Dynamo PR CI cancelled — run 39510330241 (attempt 1) on 172830710b

Gate checks: ❌ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Framework Build
SGLang ⏹️ 1

Failure details

No job failed. 1 build was cancelled (backend-status-check fails only because of it): it was not a fail-fast cancel, since nothing else failed; it stopped ~60 min after starting (22:45 → 23:45 UTC), which looks like a job time limit or a manual cancel. Rerunning the failed jobs should be enough.

⏹️ sglang-runtime / Build multi-arch cuda13.0: The operation was canceled

Job: sglang-runtime / Build multi-arch cuda13.0 · Cancelled step: Compliance extract · Logs: gh run view --job 118929612042 -R ai-dynamo/dynamo --log-failed

2026-10-20T23:45:49Z ##[error]The operation was canceled.

Cancelled during Compliance extract with no prior error in the log.

For agents
{"pr": 16947, "run_id": 39510330241, "run_attempt": 1, "head_sha": "172830710b205e1891e3d76d59abb560a3125669",
 "failures": [{"job": "sglang-runtime / Build multi-arch cuda13.0", "job_id": 118929612042, "failed_step": "Compliance extract", "signature": "cancelled: The operation was canceled (~60 min after start)", "tests": [], "log_cmd": "gh run view --job 118929612042 -R ai-dynamo/dynamo --log-failed"}]}

Posted automatically by Devin for run 39510330241. Updated on every full-CI run of this PR.

@krishung5
krishung5 merged commit 5004f3f into ai-dynamo:main Oct 21, 2026
201 of 206 checks passed
erezzarum pushed a commit to erezzarum/dynamo that referenced this pull request Oct 23, 2026
sttts pushed a commit to sttts/dynamo that referenced this pull request Oct 27, 2026

This branch was previously deployed

1 inactive deployment
external_collaborator — 17283071 Deployed Oct 20, 2026 by HanFa via ok-to-test #36168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` refactor size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants