Repository navigation
Conversation
|
👋 Hi HanFa! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. WalkthroughThe change adds a GLM5-Next image-routing backend that counts image tokens and validates prompt layouts. Preprocessing uses validation results when building multimodal routing information. Tests cover configuration resolution, routing output, and invalid inputs. ChangesGLM5-Next Image Routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk:
|
Use dynamo-multimodal 0.2.2 for checkpoint-driven GLM image token counts and layout validation. Select the adapter before legacy flat-config loading and fall back to text-prefix routing when planning is unsupported or fails. Add Rust adapter and preprocessing tests plus real Python binding resolver tests with shared lightweight checkpoint fixtures. Three-backend routing E2E remains follow-up work. Validation: targeted preprocessor Rust tests passed (383 passed, 2 ignored); Python binding tests passed (11 passed); cargo fmt, Ruff, and git diff --check passed. Signed-off-by: Fang Han <fangh@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
e3d865a to
e15298f
Compare
|
/ok to test e15298f |
|
/ok to test e15298f |
⏹️ Dynamo PR CI cancelled — run 41936721700 (attempt 1) on
|
| Framework | Deploy |
|---|---|
| vLLM | ⏹️ 1 ✅ 3 |
Failure details
No jobs failed. 1 job was cancelled: vllm Deploy Test / agg. It does not look like a fail-fast cancel, because nothing else failed. Its logs show worker pods stuck in a SyncError loop for about 25 minutes before the cancel, so either the job timed out or someone cancelled it by hand.
⏹️ vllm Deploy Test / agg: cancelled while worker pods were stuck in SyncError
Job: vllm Deploy Test / agg · Cancelled step: Run Dynamo Deploy Test · Logs: gh run view --job 126961251373 -R ai-dynamo/dynamo --log-failed
[TEST] 2026-11-05T00:48:10 INFO ManagedDeployment: vllm-agg-router-0-worker-crc7j: SyncError - Error syncing: patch host object: update object: Operation cannot be fulfilled on pods "vllm-agg-router-0-worker-crc7j-x-default-x-ci-41936721700"
[TEST] 2026-11-05T00:48:10 INFO ManagedDeployment: vllm-agg-router-0-worker-skqg8: SyncError - Error syncing: patch host object: update object: Operation cannot be fulfilled on pods "vllm-agg-router-0-worker-skqg8-x-default-x-ci-41936721700"
##[error]The operation was canceled.
The deploy step ran from 00:23:52 to 00:48:49 UTC. Throughout, the vllm-agg-router worker pods logged SyncError ... Operation cannot be fulfilled on pods on a loop until the step was cancelled. This looks like a K8s pod sync or scheduling stall, so rerunning the failed jobs is likely enough.
For agents
{"pr": 17121, "run_id": 41936721700, "run_attempt": 1, "head_sha": "da11fe54ebd0ffa375baa8061414443700caee03",
"failures": [{"job": "vllm Deploy Test / agg", "job_id": 126961251373, "failed_step": "Run Dynamo Deploy Test", "signature": "cancelled after ~25 min; worker pods SyncError 'Operation cannot be fulfilled on pods'", "tests": [], "log_cmd": "gh run view --job 126961251373 -R ai-dynamo/dynamo --log-failed"}]}Posted automatically by Devin for run 41936721700. Updated on every full-CI run of this PR.
External PR triageBlocker: CI is green on the current head, but none of the requested CODEOWNER teams has reviewed since 2026-10-22 (6 days). Only the review bot has approved. Next action (@ai-dynamo/dynamo-frontend-codeowners @ai-dynamo/dynamo-multimodal-codeowners @ai-dynamo/dynamo-router-codeowners @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-ops-codeowners): Review and approve, or request changes. Status as of 2026-10-28 for head |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Could we add postmerge E2E coverage with the tiny GLM checkpoint for vLLM and SGLang? These tests verify frontend/planner agreement, but not worker expansion or KV-event hash parity. Please check repeated-image cache overlap, different-image isolation, and multiple images across block boundaries.
There was a problem hiding this comment.
I have added tests/mm_router/test_router_rust_mm_glm5_next_e2e.py, please note that they will be run in the nightly e2e since they requires sparse MLA attention which vLLM and sglang implements for only Blackwell and Hopper.
From my agent:
One caveat on "post-merge": GLM5-Next's DSA layers need sparse MLA attention, which vLLM 0.32 (FlashMLA sparse / FA3 / FlashInfer SM90) and SGLang 0.5.22 (NSA) implement only for Hopper and Blackwell, so even the tiny checkpoint cannot run on the 24 GiB Ada runners behind the post-merge lanes. I marked the module nightly + h100 so it runs in vllm-h100-test / sglang-h100-test, and added not h100 to the nightly single-GPU vLLM lane the way the SGLang lane already has it. If you want it post-merge, that needs an H100 post-merge lane; happy to follow up if the CI owners prefer that.
There was a problem hiding this comment.
meanwhile can I have anther ok to test?
The Rust planner tests and the Python resolver tests never run an engine, so they cannot catch the glm5_next planner and a worker disagreeing on image token expansion, or a hash mismatch between the frontend's routing blocks and the worker's KV events. Add an E2E module that drives the Rust frontend's KV router against a real vLLM or SGLang worker serving tiny-random/glm-5.3-flash, a 227 MB random checkpoint that keeps GLM-5.3-Flash's tokenizer, chat template, processor config and token ids. Per engine it asserts that the router's block total equals the engine's reported prompt length after its own expansion, that per-image token increments follow GLM geometry, that repeated images match nearly every published block and hit the engine's prefix cache, that a different image on the same prompt matches neither, and that two non-block-aligned images agree block for block with the second image's blocks invalidated exactly from the boundary on. GLM5-Next's DSA layers need sparse MLA attention, which vLLM 0.32 and SGLang 0.5.22 provide only on Hopper and Blackwell, so the module is nightly + h100 and runs in the vllm-h100-test and sglang-h100-test lanes. Exclude h100 tests from the nightly single-GPU vLLM lane the way the SGLang lane already does, so they do not land on its Ada runner. Signed-off-by: Fang Han <fangh@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/ok to test 3d54251 |
|
@HanFa thanks for addressing the comments!
Could we switch to |
The first GPU run of the GLM E2E module, on GB200 in review, failed in two ways. tiny-random/glm-5.3-flash does not start: its kv_lora_rank of 256 is not a sparse-MLA head size vLLM accepts, and SGLang cannot load its 32x32 block-FP8 weights. With a BF16 checkpoint both engines serve, but they size KV blocks for GLM5-Next's hybrid KDA layers themselves (vLLM advertised 4352 tokens, SGLang 256) while the assertions assumed 64, and a 4096-token context cannot fill even one vLLM block. Switch to inference-optimization/GLM-5.3-Flash-2.6B-A2.3B-small pinned at a2d7ca22e78d. The workers load the pinned snapshot from a local path, so both engines and the frontend's planner read the same files. Read the block size the worker advertised from the frontend's model_kv_cache_block_size metric, and derive the image sizes and every block assertion from it. The large image covers about one and a half blocks and the small one just over a block, so every image prompt spans several blocks and a second image always reaches a block the first did not. Raise the context to 32768 tokens. Bound cold overlaps and engine reuse by the blocks the text alone fills, measured with a text-only request. Drop the kernel pins, which only the block-FP8 checkpoint needed, and the frontend's --kv-cache-block-size, which does not apply to discovered workers. Drop the "resolved image-placeholder token id" log assertion: GLM's placeholder comes from config.json's image_token_id, and that path does not log it. Signed-off-by: Fang Han <fangh@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ok to test 97296c2 |
Review follow-up on 97296c2. The shared predownload fetched the checkpoint's default branch before it turned on HF_HUB_OFFLINE. Once that branch moves past the pinned revision, a clean runner would not find the pinned snapshot. The model mark now takes revision=: pytest_collection_modifyitems records it as repo_id@revision and download_models fetches that revision. The GLM module passes its pin and only looks the snapshot up in the cache. In the multi-image test, AC_cold must now also match more blocks than the text-only prompt fills. The old [A repeat - 1, A repeat] bound accepted zero when the A repeat matched a single block, as it does at GB200's 4352-token vLLM block, so losing the first image's cached blocks would have passed. _send now returns a RequestObservation and checks the worker's expansion against the router's block count before returning. That removes the tuple unpacking and the per-test expansion calls. Signed-off-by: Fang Han <fangh@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ok to test da11fe5 |
|
Validated on GB200 with the pinned small GLM checkpoint and one worker per backend:
|
Summary
Connect GLM-5.3-Flash image routing to the default Rust frontend through the shared
glm5_nextplanner. Use checkpoint metadata for image token counts and validate the rendered prompt layout before constructing exact routing tokens. Unsupported configuration, accounting failures, and layout failures retain text-prefix routing.Details
dynamo-multimodaldependency to0.2.2with default features disabled; update both Cargo lockfiles.processor_config.jsoncheckpoints are supported.tests/mm_router/test_router_rust_mm_glm5_next_e2e.py: nightly H100 E2E that drives the Rust frontend's KV router against a real vLLM or SGLang worker servinginference-optimization/GLM-5.3-Flash-2.6B-A2.3B-smallpinned ata2d7ca22e78d(a 5 GB BF16 cut of GLM-5.3-Flash that keeps its tokenizer, chat template, processor config and token ids; the shared predownload fetches that revision and the workers load the snapshot from a local path). The engines size KV blocks for the model's hybrid KDA layers themselves, so the module reads the block size the worker registered from the frontend'sdynamo_frontend_model_kv_cache_block_sizemetric and derives the image sizes and every block assertion from it. Per engine it checks worker expansion (router block total vs the engine's reported prompt length, per-image increments following GLM geometry), repeated-image cache overlap plus the engine's prefix-cache hit, different-image isolation at the router and in the engine cache, and two images whose runs straddle block boundaries, where swapping the second image must keep the first image's blocks. Excludeh100tests from the nightly single-GPU vLLM lane the way the SGLang lane already does.tests/conftest.py:@pytest.mark.model(repo_id, revision=...)pins the shared predownload to that revision, so a test that loads a fixed revision finds it cached onceHF_HUB_OFFLINEis set. The collected entry readsrepo_id@revision, anddownload_modelspasses the revision tosnapshot_download. Marks withoutrevision=download the default branch as before.Context
Follow-up to #16947, now merged into
main. This PR contains the GLM integration commit, rebased onto upstreammainat4b487d890d, plus three test commits:3d54251faeadds the E2E module requested in review,97296c2c6reworks it after its first GPU run (GB200, in review), which showed that the original checkpoint does not start and that the engines do not use 64-token KV blocks for this model, andda11fe54eaddresses the review of that rework (pinned-revision predownload, a stricter two-image overlap check, a typed per-request observation). Relative to the previously reviewed revision (e3d865aba3), the only change to the integration commit is a one-line clippy fix inlib/llm/src/preprocessor/glm5_next_tests.rs: the&inCheckedFile::from_disk(&model_dir.join("config.json"))triggeredclippy::needless_borrows_for_generic_argsand failed therust-clippy (.)pre-merge job. The previous run'slycheefailure was a pre-existing 404 in the v1.5.0 release notes, fixed onmainby #17091 and picked up by the rebase.Supersedes the temporary stacked review at HanFa#3.
Where should the reviewer start?
Start with
lib/llm/src/preprocessor/mm_routing/glm5_next.rs, then the adapter selection and layout-validation call sites.lib/llm/src/preprocessor/glm5_next_tests.rsexercises the preprocessing boundary;lib/bindings/python/tests/test_routing_image_token_id.pyexercises the existing Rust resolver exposed to Python.tests/mm_router/test_router_rust_mm_glm5_next_e2e.pyis the engine-backed check of the same contract.Validation
mainat4b487d890d: the GLM integration commit (11 files; rebased without conflicts, the only change since the previously reviewed revision is the one-line clippy fix noted above), the E2E test commit (the test module plus a one-line nightly lane filter), the E2E rework (the test module only), and its review follow-up (the test module plus therevision=support intests/conftest.py).cargo fmt --all --checkandgit diff --checkpassed, andcargo metadata --locked --no-depspassed for both the workspace and the Python binding crate (both lockfiles pindynamo-multimodal0.2.2).cargo clippy -p dynamo-llm --no-default-features --features mm-routing --all-targets --no-deps -- -D warningspassed (the lint target that failed in the previous pre-merge run).cargo test -p dynamo-llm --no-default-features --features mm-routing --locked --lib preprocessor:: -- --test-threads=4: 405 passed, 2 ignored.maturin develop --uv --features mm-routing --locked(Python 3.12, abi3 wheel), then ranpython -m pytest lib/bindings/python/tests/test_routing_image_token_id.py -q: 11 passed.97296c2c6:pytest --collect-onlycollects 6 tests (3 per engine); the nightlyvllm-h100-test/sglang-h100-testmarker filters select 3 each, and the non-H100 nightly lane filters and thepost_mergeandpre_mergemarkers select none. The repository pre-commit hooks pass on it (isort, black, flake8, ruff, codespell, pytest marker report).da11fe54e: the pre-commit hooks pass,pytest --collect-onlystill collects 6 tests, and the collected download list carriesinference-optimization/GLM-5.3-Flash-2.6B-A2.3B-small@a2d7ca22…. Against the Hub, predownloading the default branch and then resolving an older pinned commit (the repo's initial commit) offline fails withLocalEntryNotFoundError; predownloading the pinned revision resolves it offline. In the cache model above, the new two-image check passes all 1185 block-size and text-length cases. A router bug that keys each image by every image in the request, so that a new second image also changes the first image's blocks, fails only the new check, in all 1185 cases; the old bound caught it in 66.3d54251faeon GB200:tiny-random/glm-5.3-flashdid not start (vLLM rejected its sparse-MLA head size, and SGLang failed to load its block-FP8 weights). With a BF16 checkpoint both engines served, but they advertised 4352-token (vLLM) and 256-token (SGLang) KV blocks against the module's assumed 64.97296c2c6andda11fe54ehave not run on a GPU yet: GLM5-Next's DSA layers need the sparse MLA attention that vLLM 0.32 and SGLang 0.5.22 implement only for Hopper and Blackwell, which rules out the 24 GiB Ada runners behind the post-merge lanes and my RTX 5090 (SM120). It carries noprofiled_vram_gibmarker yet and runs in the sequential GPU stage until profiled on an H100. The checkpoint is BF16, so the FP8 kernel pins are gone and both engines run their default kernels.The nightly E2E covers vLLM and SGLang worker expansion and KV-event hash parity with the small BF16 checkpoint. TensorRT-LLM coverage and the supported runtime/checkpoint matrix with the full model remain follow-up work; this PR does not claim full backend acceptance.
Related Issues
Part of #16948.
Internal tracking: DIS-3123 (multimodal-aware KV routing model support).
Consumes the shared planner from ai-dynamo/frontend-crates#412, released in
dynamo-multimodal0.2.2.Summary by CodeRabbit