Repository navigation
chore(container): minimax m3 builds - #14856
Conversation
|
/ok to test 72e11d5 |
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
72e11d5 to
f6c6bc1
Compare
|
/ok to test f6c6bc1 |
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test 76b18bf |
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test 2cb3b12 |
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test 81a496c |
|
/ok to test 6c8d603 |
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test e423a0f |
@NVShreyas, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test e423a0f |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at e423a0fde5fa1ff5443687a1d1bbd0ad5afe9e06. This commit is empty and has the same tree as 6c8d6038e9. At this head, I rendered the Dockerfiles again (identical), compared the list of changed files (identical), and ran git merge-tree with 95cadce97d (clean). No P0 or P1 is open. 3 P2 and 1 P3 are open.
- [P2] New: only the four PR build jobs get the vLLM base override, so PR CI does not test the image that this branch ships. Inline at
.github/workflows/pr.yaml:390. - [P2] Still open: only the last patch can fail the patch step. The loop at
container/templates/vllm_runtime.Dockerfile:207did not change. - [P2] Still open for the image that PR CI builds:
components/src/dynamo/frontend/prepost.py:22importsvllm.entrypoints.openai.engine, which vLLM8a728663does not have. The v0.28.0 default has this module. If v0.28.0 is the intended base, this item closes. - [P3] New:
RUNTIME_IMAGE_TAGat.github/workflows/pr.yaml:391names a tag. If someone pushes that tag again, the base changes without a change in this repository. You can pin the digest in the same argument:RUNTIME_IMAGE_TAG=vllm-nightly-8a728663c1c3@sha256:f5df5cc3302b5f404848c4eca88d7bf7ed5226e151c056da22816d7734644d67. I tested this form in BuildKit with the sameARGandFROMlines and public images. - Fixed: my earlier P1. The mirror tag resolves to the digest of the deleted public image. I replied on that thread.
- Fixed: my earlier P3. The description now says
tokenizers==0.22.2.
CI on this head at 22:31Z: in run 37539454283 (PR), vllm-dev, vllm-local-dev and vllm-runtime-efa passed, and vllm-runtime still ran. The vLLM test jobs had not started. Pre Merge (run 37539292448) still ran.
What I measured.
- I rendered the vLLM
runtime,dev,local-devand EFA Dockerfiles. All four declareARG RUNTIME_IMAGEandARG RUNTIME_IMAGE_TAGbefore the firstFROM, so the build arguments inpr.yamlreachFROM. - In run 37539454283, the three finished vLLM jobs built from
vllm-base:vllm-nightly-8a728663c1c3@sha256:f5df5cc3302b5f404848c4eca88d7bf7ed5226e151c056da22816d7734644d67. This is the digest of the deleted public image. - I ran the rendered patch step and a
--dry-runof each patch invllm/vllm-openai:v0.28.0-ubuntu2404andvllm/vllm-openai:v0.30.0-ubuntu2404. The8a728663files were the control. - Since my last review, only
.github/workflows/pr.yamlandcontainer/context.yamlchanged. I did not run the router and scheduler tests again, because their code did not change.
Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test e01c0aa |
There was a problem hiding this comment.
This PR includes changes beyond MiniMax support. Are these general bug fixes that should go to main and then be cherry-picked here? Two examples are forwarding image build arguments to compliance extraction and correcting the load accounting for remote-KV waits in the vLLM waiting queue.
Separately, is the core router prefill-accounting change fixing a bug? Please explain the failure it addresses and the validation behind it. If it is a general fix, it should probably go through a separate PR to main.
|
Thanks for working on this, @NVShreyas . My naive thinking is dynamo relies on upstream release image so that we don't need patches for a specific model. Could you help me understand what motivates this PR? Is it okay for dynamo |
@jthomson04 yes those should go into main. I already have/will open PRs for the necessary ones. I'm not sure if they should be merged before and then cherry-picked here cause of the timelines. In the past, we've always added patches here and then opened separate PRs to get changes merged to main, so as to not block recipe release. |
@furionw Yes. The motivation for container build PRs (not just this one, but in general for recipes) is as follows:
I do understand your point though. In an ideal case, we would merge to upstream first and then update dynamo. Right now it might not be possible to keep up with the recipe releases. Please feel free to bring this up internally so we can have a wider chat if needed |
Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com> Signed-off-by: NVShreyas <158103197+NVShreyas@users.noreply.github.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 09af0588991425f67886a37e9eebb1f17cfff13b. No P0 or P1 is open. 2 P2 and 1 P3 are open. The last commit applies my suggestion, so I partly review my own line.
- [P2] New: with
ENABLE_VLLM_OMNI=false, the nightly images have noscipy, so the realtime audio worker fails. Inline atcontainer/deps/vllm/install_vllm_omni.sh:10. - [P2] Partly fixed: post-merge and the other non-PR builds still use
v0.28.0. I replied on the thread at.github/workflows/pr.yaml:390. - [P3] Still open:
RUNTIME_IMAGE_TAGat.github/workflows/pr.yaml:391names a tag, not a digest. - Fixed: the
prepost.py:22import (I resolved that thread), and the patch loop, which now stops at the first failing patch. - Not graded, from other reviewers:
jthomson04asks that the general fixes go tomainfirst, andfurionwasks why this branch needs patches. The author answered both.
CI: on this head, only the source-branch checks ran, and Pre Merge (run 37559165494) passed. The PR workflow last ran on e01c0aa638 (run 37544955772), one Dockerfile line behind this head. The vLLM image builds and tests of this head did not run.
What I measured.
- CI code. I ran both changed
run:scripts of.github/actions/compliance-extract/action.ymlat the base and at this head. I used GitHub's shell flags, bash 3.2 and 5.3, and a stubdocker. Ifextra_build_argsis empty or unset, both steps pass the same arguments as on the base. The threepr.yamllines become three--build-argitems. A line with a space or a*stays one item. All 7 new${{ }}lines are underenv:orwith:, not inrun:. - Callers. In
pr.yaml, onlyvllm-runtimeandvllm-runtime-efareach the extract with a value, because the extract skipsdevandlocal-dev.post-merge-ci.yml,nightly-ci.yml,build-on-demand.yml,dynamo-pipeline.ymlandshared-build-sidecar.ymlpass no value. ENABLE_VLLM_OMNI. The rendered Dockerfiles of this head differ frome423a0fde5by three lines per target.ARG ENABLE_VLLM_OMNI=truecomes before the firstFROM, and the script also defaults totrue. So a build that does not pass the argument keeps the old install path. Invllm/vllm-openai:v0.28.0-ubuntu2404,falseexits 0 and removes an inheritedvllm-omni. Any value other thantrue,falseor empty exits 1.- Tests. In Dynamo test images with vLLM 0.28.0 and 0.30.0, I ran 8 test files at this head and at
e423a0fde5, with--network none. On 0.28.0 both trees give the same counts. On 0.30.0, every file except the one below passes at this head, ande423a0fde5has 4 collection errors and 3 failures.test_vllm_processor_unit.pyhas 117 errors on both trees in both images, because it needsQwen/Qwen3-0.6Bfrom Hugging Face. CI passed all 160 of its tests one01c0aa638. git merge-tree --write-treewith the base tip95cadce97dis clean, and its tree is the tree of this head.
|
Why is the core router prefill-accounting change included in this MiniMax image PR? What problem in the recipe requires it? |
@jthomson04 This fixes the prefill-load accounting used by the MiniMax agg recipe with CPU KV offloading. The router discounts host/disk cache hits when scoring transfer cost, then reuses that discounted overlap to estimate GPU prefill work. That makes cached tokens count as compute and could steer requests away from the worker holding their KV. On a separate note, the |
Fall back to container duration when the video stream duration is unknown. Cover the reported finalized VP9 WebM through the real decoder with all-frame, FPS-based, and fixed-frame sampling. Fixes #15811 Signed-off-by: Shreyas Misra <shreyasm@nvidia.com>
|
/ok to test e46c791 |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at e46c79195aa3b138aa99e4a3e1cbddb0f4c54b14. My approval moves from 09af058899 to this head. No P0 or P1 is open. 2 P2 and 2 P3 are open. Commit 09af058899 applies my suggestion, so I partly review my own line.
- [P2] Still open: with
ENABLE_VLLM_OMNI=false, the nightly images have noscipy, so the realtime audio worker fails. Inline atcontainer/deps/vllm/install_vllm_omni.sh:10. Run 37656785363 on this head fails the same 3 realtime tests on amd64 and arm64. - [P2] Still open: post-merge and the other non-PR builds still use
v0.28.0. My reply is on the.github/workflows/pr.yaml:390thread. - [P3] Still open:
RUNTIME_IMAGE_TAGat.github/workflows/pr.yaml:391names a tag, not a digest. - [P3] New: git treats the new fixture
lib/llm/tests/data/media/224p_6_vp9.webmas a text file. The*line of.gitattributessetstext eol=lf, and no line marks*.webmasbinary. The fixture is intact now. But if a new version of it holds a CR LF byte pair,git addremoves the CR and gives only a warning. Please add a*.webm binaryline next to the*.mp4line. - Not graded: the CHANGES_REQUESTED review of
jthomson04stands.
CI on this head: no job ran the new test. Pre Merge (run 37656340383) passed, but media-ffmpeg is not a default feature, so it built no video decoder test. In the PR workflow (run 37656785363), dynamo-runtime / rust-gpu was skipped, because its image build stopped at the 60-minute limit.
What I measured on the new commit.
- Scope. From
09af058899to this head, onlylib/llm/src/preprocessor/media/decoders/video.rschanged, andlib/llm/tests/data/media/224p_6_vp9.webmis new. The merge base is still the base tip95cadce97d. - The fixture has 846 bytes, sha256
94492c00390c27381a47d9bfbb955257204696de2a82a9c35bcc01d4a5e49232. I builtffprobe8.1.2 with the same demuxers as the in-tree FFmpeg. It shows one VP9 video stream, 224x224 at 2 fps. The stream has 6 packets and 6 decoded frames (1 I and 5 P) from 0 to 2.5 s. The stream has no duration and no frame count. The container duration is 3.000000 s. The file has no attachments, chapters or other streams. The Matroska segment ends at byte 846, so no data follows it. - Tests. I used a
dynamo-runtime-testimage, the kind that therust-gpujob uses, built from5a28947ec6. It has Rust 1.96.1 and the in-tree FFmpeg (libavformat 62.12.102). At this head,cargo test --locked -p dynamo-llm --no-default-features --features media-ffmpeg --lib preprocessor::media::decoders::video::testspasses 17 of 17 tests. - Mutation. I made the fallback compute the stream duration again, as before this commit. All 3 cases of
test_decode_webm_with_container_durationthen fail withCannot determine the video frame count. After I restored the file (same sha256), 17 of 17 pass again. - With
media-ffmpeg,cargo clippywith-D warningsandcargo fmt --checkpass fordynamo-llm. A lint and a format error that I planted in the new test made each of them fail. .gitattributes. I used a scratch repository with the.gitattributesof this head. There, 14 bytes that hold one CR LF pair became a 13-byte blob ast.webm, and stayed 14 bytes ast.mp4. The committed fixture has no CR LF pair.
7f10471
into
release/1.6.0-minimax-m3-dev.1
Summary
Build MiniMax M3 vLLM runtime, dev, local-dev, and EFA images with
RUNTIME_IMAGE=${{ vars.ECR_REGISTRY }}/ai-dynamo/vllm-baseandRUNTIME_IMAGE_TAG=vllm-nightly-8a728663c1c3in PR build arguments. Preserve the base branch's image defaults incontainer/context.yamland retain itstokenizers==0.22.2compatibility fix.Explicitly exclude incompatible vLLM-Omni from these images with
ENABLE_VLLM_OMNI=false, removing inherited installations as well as skipping installation. Other builds retain the enabled default. Forward image build overrides to compliance and source extraction so they describe the same image.Support the nightly's moved frontend protocol types while retaining the released layout, and align API contract tests with supported KV-event metadata, optional speculative metrics, and actual scheduler-output construction.
Include vLLM offloading, MiniMax M3 structured-output and patch-embedding fixes, and compute-only prefill load accounting.
Fix frontend decoding of finalized VP9/WebM inputs with unknown stream duration/frame count by falling back to valid container duration. Add the exact reported clip as a regression fixture (Fixes fix(frontend): use container duration for WebM video decoding #15811).
Validation
Reproduced the three scheduler/KV-event contract failures in a local image containing the pinned vLLM
8a728663crevision.After the fixes, 32 API contract tests and 18 frontend pre/post-processing tests passed in that local image. Frontend tests used a previously built local Dynamo runtime wheel; this is not a full reproduction of the CI image.
Verified protocol fallback and propagation of missing internal dependencies, removal of an inherited Omni distribution, and repeated exclusion when Omni is already absent.
Rendered runtime, dev, local-dev, and EFA multi-architecture Dockerfiles. Verified all four PR build overrides and literal build-argument forwarding through compliance/source extraction.
Pre-commit on changed files and
git diff --checkpassed. Full image builds and GPU validation are delegated to CI.WebM regression: all three sampling cases fail with
Cannot determine the video frame countbefore the fix and pass afterward. All 17 video-decoder tests passed withcargo test -p dynamo-llm --no-default-features --features media-ffmpeg --lib preprocessor::media::decoders::video::testsin the local FFmpeg 8.1.2 wheel-builder container. This validates the real decoder, not rebuilt-image HTTP serving; streaming/non-streaming HTTP retesting is pending an image rebuild.