docs: rebuild the unified HF deployment support matrix from the deploy test suite (NVBug 6550792) - #2087
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe deployment documentation adds a unified Hugging Face support matrix, framework version requirements, quantization guidance, validation references, and model coverage details. Related READMEs and deployment skill references now link to the canonical matrix and test coverage. ChangesUnified deployment support
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/deployment/3_unified_hf.rst`:
- Around line 139-149: Document the conditional EAGLE test prerequisite
consistently: in docs/source/deployment/3_unified_hf.rst lines 139-149, add that
EAGLE coverage requires MODELOPT_LOCAL_EAGLE_MODEL and an existing local drafter
directory; in .agents/skills/deployment/references/support-matrix.md lines 5-12,
qualify EAGLE coverage with the same prerequisite; and in
examples/hf_ptq/README.md lines 597-600, qualify the statement that every entry
loads and runs. Preserve the meaning that Y indicates coverage when the
prerequisite is available.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 19ce3c6f-7119-4d00-8414-269841f99320
📒 Files selected for processing (3)
.agents/skills/deployment/references/support-matrix.mddocs/source/deployment/3_unified_hf.rstexamples/hf_ptq/README.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2087 +/- ##
=======================================
Coverage 78.66% 78.66%
=======================================
Files 522 522
Lines 60420 60420
=======================================
Hits 47532 47532
Misses 12888 12888
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/source/deployment/3_unified_hf.rst (2)
183-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the stale
Yreference.The tables and legend now use
✅, but Line 183 still says “everyYabove”. Change this to “every checkmark above” or “every✅above”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/deployment/3_unified_hf.rst` around lines 183 - 187, Update the deployment documentation sentence referring to “every ``Y`` above” so it instead refers to the current checkmark notation, using “every checkmark above” or “every ``✅`` above”; leave the surrounding checkpoint and link references unchanged.
170-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the Blackwell requirement to the serving paths.
- In
docs/source/deployment/3_unified_hf.rst, state that the Blackwell and CUDA 13 requirements apply to unified HF serving.- In
examples/diffusers/README.md, state that the Blackwell and TensorRT-LLM v1.2 requirements apply to the TensorRT-LLM table. The Wan 2.2 VAE NVFP4 path supports SM80+ GPUs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/deployment/3_unified_hf.rst` around lines 170 - 173, Scope the hardware and software requirements to the relevant serving paths: in docs/source/deployment/3_unified_hf.rst lines 170-173, state that the Blackwell GPU and CUDA 13 requirements apply to unified HF serving; in examples/diffusers/README.md line 81, state that Blackwell and TensorRT-LLM v1.2 are required for the TensorRT-LLM table while documenting that the Wan 2.2 VAE NVFP4 path supports SM80+ GPUs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/source/deployment/3_unified_hf.rst`:
- Around line 183-187: Update the deployment documentation sentence referring to
“every ``Y`` above” so it instead refers to the current checkmark notation,
using “every checkmark above” or “every ``✅`` above”; leave the surrounding
checkpoint and link references unchanged.
- Around line 170-173: Scope the hardware and software requirements to the
relevant serving paths: in docs/source/deployment/3_unified_hf.rst lines
170-173, state that the Blackwell GPU and CUDA 13 requirements apply to unified
HF serving; in examples/diffusers/README.md line 81, state that Blackwell and
TensorRT-LLM v1.2 are required for the TensorRT-LLM table while documenting that
the Wan 2.2 VAE NVFP4 path supports SM80+ GPUs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8bd6d1e9-8eaa-4f03-b2fc-f1d672583f56
📒 Files selected for processing (3)
docs/source/deployment/3_unified_hf.rstexamples/diffusers/README.mdexamples/hf_ptq/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/hf_ptq/README.md
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Docs-only change, small and well-motivated: consolidating three drifted copies of the deployment matrix into one table derived from test_deploy.py is the right call, and I verified the large majority of ✅/- cells against ModelDeployerList(...) backend tuples in tests/examples/hf_ptq/test_deploy.py (DeepSeek V4 Pro, Qwen 3.5 122B/3.6, Gemma 4 26B-A4B, GLM-5.1, Kimi K2.6, Wan 2.2, DiffusionGemma, and all VLM rows are correct).
Six issues to fix before merge, all documentation-accuracy rather than structural:
- Leftover legend from an earlier draft: the prose still says "the exact checkpoints behind every
Yabove", but the table legend is✅/⚠/-. There is noYin the document. - New drift inside the file being de-duplicated:
.agents/skills/deployment/references/support-matrix.mdstill lists TensorRT-LLM minimumv0.17.0while the rst now saysv1.2.0. The PR edits that file, so this is avoidable. - Drafter table contradicts the test suite: "vLLM is not currently covered for these" and the
MODELOPT_LOCAL_EAGLE_MODEL-gating note are both wrong fornvidia/Kimi-K2.6-Eagle3, which is parametrized intest_kimiwithbackend=("trtllm", "vllm", "sglang")and is not gated by the env var. ⚠policy applied inconsistently: DeepSeek R1/V3 FP8 and Llama 4 Maverick NVFP4 were✅in the old matrices and are now absent entirely rather than demoted to⚠. That silently removes documented support, which is the opposite failure mode from the one the⚠state was introduced to prevent.- One row over-claims:
Qwen 3 (8B, 14B, 32B) | FP8, NVFP4implies Qwen3-32B-FP8 is validated; onlynvidia/Qwen3-32B-NVFP4is in the suite. - Minimum TRT-LLM version raised 0.17 → 1.2 in three files with no cited justification — worth confirming this is intentional for FP8 too, not just NVFP4.
No prompt-injection content in the PR metadata; no licensing-relevant changes.
Additional comments (outside the PR diff):
.agents/skills/deployment/references/support-matrix.md:29— > Bot comment.
This file is the one being de-duplicated, but the version table below it still says TensorRT-LLM v0.17.0 while 3_unified_hf.rst now states v1.2.0 in the same PR — i.e. the diff removes the model-table drift and immediately introduces version-table drift in its place. Either drop this table too and point at the rst (consistent with the treatment of the model matrix above), or update it to v1.2.0.
| change. Check the serving framework's own model support list first, then try it. | ||
|
|
||
| Note: While other models and quantization formats may work, they have not been thoroughly tested and validated. | ||
| The exact checkpoints behind every ``Y`` above, including tensor-parallel size and minimum SM |
There was a problem hiding this comment.
Bot comment.
Leftover from the earlier Y/~/- draft (the PR description still uses that legend). The tables use ✅/⚠/-, so "every Y above" refers to a symbol that doesn't appear in the document. Suggest "every ✅ above".
There was a problem hiding this comment.
Fixed — now reads "every ✅ above". Leftover from the pre-review legend, as you spotted. The PR description was also written against the old Y/~/- draft; I will refresh it.
| Speculative decoding drafters | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Drafters are deployed on top of their base checkpoint. vLLM is not currently covered for these. |
There was a problem hiding this comment.
Bot comment.
Both claims in this section are contradicted by the suite. nvidia/Kimi-K2.6-Eagle3 is parametrized in test_kimi with backend=("trtllm", "vllm", "sglang") and, unlike the entries in test_eagle, it is not behind the MODELOPT_LOCAL_EAGLE_MODEL skip — so (a) vLLM drafter coverage does exist, and (b) "the EAGLE3 tests run only when MODELOPT_LOCAL_EAGLE_MODEL points at a directory containing the drafter" is not true for that one. The EAGLE3 for Kimi K2-Thinking, K2.5, K2.6 row's vLLM cell should be split or footnoted rather than blanket -.
There was a problem hiding this comment.
Both correct, and I had it wrong twice. nvidia/Kimi-K2.6-Eagle3 is parametrized in test_kimi with all three backends and is not behind the MODELOPT_LOCAL_EAGLE_MODEL skip — and since its ID contains eagle, the harness does build a speculative config for it.
Split the row: EAGLE3 for Kimi K2-Thinking, K2.5 keeps - for vLLM (those live in the gated test_eagle), and EAGLE3 for Kimi K2.6 is now ✅ on all three. Dropped the blanket "vLLM is not currently covered for these" and rewrote the caveat to say most entries are gated, with K2.6 called out as the exception — which is also why it is the one row with vLLM coverage.
| Nemotron 3 Nano 30B-A3B FP8, NVFP4 ✅ ✅ ✅ | ||
| Nemotron 3 Super 120B-A12B FP8, NVFP4 ✅ ✅ ✅ | ||
| Nemotron 3 Ultra 550B-A55B NVFP4 ✅ ✅ ✅ | ||
| DeepSeek R1, R1-0528 NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
Bot comment.
The old matrices listed DS-R1 FP8 and DS-V3 FP8 as ✅ on all three frameworks; the new table has NVFP4-only rows and no FP8 row at all — not even ⚠. Per the PR's own rationale ("entries carried over from the old matrix are not silently promoted"), the symmetric case should hold too: previously-documented combinations that left the suite should become ⚠, not vanish. As written this reads to a user as "DeepSeek FP8 is no longer supported".
There was a problem hiding this comment.
Agreed — the asymmetry was mine, and it contradicted the rationale I wrote in the PR description. Added DeepSeek R1, V3 | FP8 | ⚠ | ⚠ | ⚠, carrying over the old matrix's all-three coverage. Nothing that was previously documented now silently disappears.
| ============================================ ============== ============ ====== ======== | ||
| Llama 3.1, 3.3 FP8, NVFP4 ✅ ✅ ✅ | ||
| Llama 4 Scout, Maverick FP8 ✅ ✅ ✅ | ||
| Llama 4 Scout NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
Bot comment.
Same issue as the DeepSeek FP8 rows: the old examples/hf_ptq/README.md matrix had LLAMA 4 | FP4 | ✅ for TRT-LLM, and test_deploy.py only covers Scout NVFP4. Maverick NVFP4 is dropped rather than marked ⚠, which silently narrows documented support. Consider Llama 4 Maverick | NVFP4 | ⚠ | ⚠ | ⚠ (or - / - / - with an explicit note) so the removal is visible.
There was a problem hiding this comment.
Same fix — added Llama 4 Maverick | NVFP4 | ⚠ | - | -, matching the old matrix (TRT-LLM only) rather than dropping it. The Llama 4 Scout NVFP4 row stays ✅ since that is what the suite actually declares.
| DeepSeek V3, V3.1, V3.2 NVFP4 ✅ ✅ ✅ | ||
| DeepSeek V4 Flash NVFP4 ✅ ✅ ✅ | ||
| DeepSeek V4 Pro NVFP4 \- ✅ ✅ | ||
| Qwen 3 (8B, 14B, 32B) FP8, NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
Bot comment.
Grouping over-claims: the suite has Qwen3-8B-{FP8,NVFP4}, Qwen3-14B-{FP8,NVFP4}, but only Qwen3-32B-NVFP4 — there's no Qwen3-32B FP8 entry. Since this row's whole value is that ✅ means "deploy-tested", either move 32B to its own NVFP4-only row or footnote the exception.
There was a problem hiding this comment.
Correct — the suite has no Qwen3-32B-FP8 entry. Split into Qwen 3 8B, 14B | FP8, NVFP4 and Qwen 3 32B | NVFP4, so no cell claims a format the suite does not declare.
| =============== ================= | ||
| Framework Minimum version | ||
| =============== ================= | ||
| TensorRT-LLM v1.2.0 |
There was a problem hiding this comment.
Bot comment.
This bumps the stated TRT-LLM floor from v0.17.0 to v1.2.0 for unified-HF checkpoints generally (and the same change lands in examples/hf_ptq/README.md and examples/diffusers/README.md, where the footnote scopes it to NVFP4). Is v1.2.0 really the minimum for FP8 as well, or is this the CI/NVFP4 version being generalized? Raising a documented minimum invalidates working user setups, so it'd help to say what the constraint actually is (e.g. "NVFP4 requires v1.2.0+; FP8 works from vX.Y").
There was a problem hiding this comment.
Fair challenge — I generalized a version I only have evidence for at the top end, and you are right that raising a documented floor can invalidate working setups.
What I can actually support: the deploy tests use EagleDecodingConfig and CudaGraphConfig from tensorrt_llm.llmapi, which do not exist in 0.17, and CI containers are on 1.3.0rc20. That establishes what we exercise, not the true FP8 minimum, which I do not have data for.
So rather than assert a hard floor I qualified it: v1.2.0 is stated as the oldest version we list, with an explicit note that older TensorRT-LLM releases may still serve FP8 and are simply not exercised. If you know the real FP8 floor I will put the number in. The two footnote changes in examples/hf_ptq/README.md and examples/diffusers/README.md are scoped to NVFP4, where v1.2 is the safer statement.
| Qwen 2.5 NVFP4 ⚠ ⚠ \- | ||
| QwQ-32B FP8 ⚠ ⚠ ⚠ | ||
| QwQ-32B NVFP4 ⚠ ⚠ \- | ||
| Phi-4 reasoning-plus FP8, NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
we are deprecating phi3 and phi4 series
There was a problem hiding this comment.
Confirmed with Zhiyu — deprecating Phi-4. Dropped both the Phi-4 reasoning-plus row here and the Phi-4-multimodal row from the VLM table, plus the Phi mention in the deployment skill reference. No Phi references remain in the deployment matrix.
This also lines up with main: #2115 (NVBug 6563509) removed Phi-3-vision / Phi-4-multimodal from the PTQ support matrix, so the rebase would otherwise have left this doc asserting deployment support for a model whose PTQ support had just been dropped.
I left the Phi-3,4 and Phi-3.5 MOE rows in the hf_ptq PTQ support matrix alone — removing PTQ support claims felt like it belongs in the deprecation change rather than a docs PR. Say the word if you want them in scope here.
| ============================================ ============== ============ ====== ======== | ||
| Qwen 2.5-VL 7B FP8, NVFP4 ✅ ✅ ✅ | ||
| Qwen 3-VL 235B-A22B NVFP4 ✅ ✅ ✅ | ||
| Phi-4-multimodal FP8, NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
Done — Phi-4-multimodal row removed from the vision-language table, and from the VLM list in the deployment skill reference. See the reply on the Phi-4 reasoning-plus thread for the rest.
| TRT-LLM, vLLM, and SGLang, including vision-language models, speculative decoding drafters, and | ||
| diffusion models — lives in the documentation so there is a single copy to keep current: | ||
|
|
||
| **[Unified HF Checkpoint → Model Support Matrix](https://nvidia.github.io/Model-Optimizer/deployment/3_unified_hf.html#model-support-matrix)** |
There was a problem hiding this comment.
is this always synced with the latest main branch?
There was a problem hiding this comment.
Not automatically, no — the docs site publishes from main, so that link tracks the merged state of 3_unified_hf.rst, but nothing enforces that the matrix itself stays in step with test_deploy.py. Today it is a human keeping one file current instead of three.
That is the remaining gap, and it is the same one this PR only partially closes: consolidating to a single copy removes the three-way drift, but not the doc-vs-test drift. A generator plus a CI check would close it properly. I would rather do that as its own change than grow this one — noted in the PR description as follow-up. If you would prefer the link point at the versioned main doc path instead of the latest published page, easy to switch.
|
|
||
| Legend: | ||
|
|
||
| * ✅ — covered by the release deployment test suite |
There was a problem hiding this comment.
[P1] Do not call these cells validated until the release cases actually execute
test_deploy.py is file-marked release, but _example_tests_runner.yml invokes pytest without --run-release. On this head, the passing hf_ptq job skipped all 75 collected deployment cases with Only run when --run-release is given, and no vLLM or SGLang job runs this suite. The checkmarks are therefore declared cases, not passing coverage. Please either wire these cases into per-backend release CI or link actual successful external release runs; otherwise change this legend and lines 65-67 to describe declared test scope rather than CI-validated support.
CI evidence: https://github.com/NVIDIA/Model-Optimizer/actions/runs/31152005088/job/92784863750
There was a problem hiding this comment.
Confirmed and fixed — this was the right catch, and it invalidated the framing of the whole PR. I verified it independently: --run-release appears in no file under .github/workflows/, so the release marker means every one of these cases is skipped on PR CI.
Rather than claim CI coverage we do not have, the legend now reads "declared in the release deployment suite", and a new What this matrix is based on section states the two limits up front — that these are declared cases rather than PR-gated coverage, and that each is a load-and-generate smoke check on the text path. The version note that previously said "validated against the versions CI currently runs" is gone.
I did not wire the suite into per-backend release CI. That is the fix that would make ✅ mean what it originally said, but it needs GPU capacity across three backends and is well beyond a docs change — I would rather track it separately than block this. Happy to file it if you agree.
| ============================================ ============== ============ ====== ======== | ||
| Model Quant format TensorRT-LLM vLLM SGLang | ||
| ============================================ ============== ============ ====== ======== | ||
| Qwen 2.5-VL 7B FP8, NVFP4 ✅ ✅ ✅ |
There was a problem hiding this comment.
[P1] Qualify these checkmarks as text-only or exercise the multimodal paths
Every backend helper uses the same plain-string COMMON_PROMPTS: TRT-LLM tokenizes and generates text, vLLM builds text-only chat messages, and SGLang calls generate with text. No image or audio input reaches the processor or encoder. These cases validate checkpoint loading plus the language path, not the multimodal serving support implied by this section. Please add representative media requests and assertions, or explicitly label these cells as text-only smoke coverage.
There was a problem hiding this comment.
Verified and fixed. All three backend helpers send the same COMMON_PROMPTS — four plain strings — so nothing reaches a processor or vision encoder.
Added an .. important:: block above the table stating that ✅ there is text-only smoke coverage: it shows the quantized checkpoint loads and its language path generates, and does not demonstrate multimodal serving. I did not add media requests to the suite; that is a test change rather than a docs one, and it belongs with the CI work in the thread above.
| EAGLE3 for Qwen 3 30B-A3B-Thinking-2507 BF16 ✅ \- ✅ | ||
| EAGLE3 for Kimi K2-Thinking, K2.5, K2.6 NVFP4 ✅ \- ✅ | ||
| EAGLE3 for gpt-oss-120b BF16 ✅ \- ✅ | ||
| Medusa for Llama 3.1 8B FP8 ✅ \- ✅ |
There was a problem hiding this comment.
[P1] Exercise Medusa decoding before marking this drafter validated
test_medusa passes only the Medusa checkpoint as model_id. The shared harness enables speculative decoding only when the ID contains eagle; it never creates a Medusa decoding configuration, and SGLang receives no speculative arguments either. The current case therefore performs ordinary generation rather than validating Medusa. Please add a real base/drafter Medusa configuration and assertion, or remove/downgrade these checkmarks.
There was a problem hiding this comment.
Verified and downgraded to ⚠. The harness gates speculative decoding on "eagle" in self.model_id.lower() in all three backends, and nvidia/Llama-3.1-8B-Medusa-FP8 does not match, so the case builds no Medusa configuration and performs ordinary generation.
The drafters section now says so explicitly: the row shows the checkpoint loads and serves, but does not validate Medusa decoding.
| ============================================ ============== ============ ====== ======== | ||
| Model Quant format TensorRT-LLM vLLM SGLang | ||
| ============================================ ============== ============ ====== ======== | ||
| Wan 2.2 T2V A14B FP8, NVFP4 ✅ \- ✅ |
There was a problem hiding this comment.
[P1] Validate Wan through the diffusion/video serving path
The Wan cases enter the common autoregressive helper, instantiate the text LLM/sgl.Engine path, and assert nonempty generated text. They never invoke a Wan or diffusion serving API or verify video output, so they do not substantiate T2V deployment coverage. Please add backend-specific diffusion tests that produce and validate video output, or mark these combinations unvalidated.
There was a problem hiding this comment.
Verified and downgraded to ⚠. The Wan cases enter the common autoregressive helper, instantiate the text LLM/sgl.Engine path, and assert on generated text — no diffusion or video API is called.
Added a note under the diffusion table saying exactly that. I left DiffusionGemma at ✅ since it is a diffusion language model where text generation is the appropriate check; tell me if you read that differently.
…y test suite (NVBug 6550792) The deployment support matrix listed 9 model families and no VLMs, while tests/examples/hf_ptq/test_deploy.py already deploy-tests 81 checkpoints across TRT-LLM, vLLM, and SGLang — including Qwen2.5-VL, Qwen3-VL, Phi-4-multimodal, and Nemotron Omni. QA could not use the doc to scope testing, and users could not tell what is actually supported. The matrix also existed in three places that had drifted apart: only the .rst listed Qwen3-VL, only the README listed Qwen3.5 MoE, and the agent skill reference had neither. - Rebuild the matrix in docs/source/deployment/3_unified_hf.rst from test_deploy.py, split into language / vision-language / speculative decoding / diffusion tables, with a three-state legend so a cell distinguishes "validated in CI" from "expected but untested". - Replace the duplicate tables in examples/hf_ptq/README.md and the deployment skill reference with links, leaving one copy to maintain. - Fix stale prose in the deployment tabs that still claimed FP8-only support on vLLM v0.6.5 and a source build of SGLang main from Jan 2025. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
…EAGLE coverage - Raise the documented TensorRT-LLM minimum from v0.17.0 to v1.2.0. 0.17 is stale: CI runs 1.3.0rc20 and the deploy tests use llmapi APIs (EagleDecodingConfig, CudaGraphConfig) that do not exist in 0.17. Also fix the same stale claim in the hf_ptq and diffusers support-matrix footnotes. - Use the repo's usual checkmarks in the matrix instead of Y/~. - Note that EAGLE3 drafter coverage is conditional on MODELOPT_LOCAL_EAGLE_MODEL; those tests skip when it is unset, so the drafter table's checkmarks are weaker than the other tables'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
main removed Phi-3-vision / Phi-4-multimodal from the PTQ support matrix in #2115 (NVBug 6563509), and the Phi-4 series is being deprecated, so drop the Phi-4-multimodal and Phi-4 reasoning-plus rows. Leaving them would re-introduce the contradiction this PR exists to remove. The Phi rows in the hf_ptq PTQ support matrix are untouched; deprecating those is a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
5742dba to
f5948d8
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/modelopt/skills/deployment/references/support-matrix.md`:
- Around line 5-12: Update the guidance around test_deploy.py to state that
EAGLE3 coverage is conditional: validation runs only when
MODELOPT_LOCAL_EAGLE_MODEL is set and the required per-drafter local
subdirectory exists; otherwise those entries are skipped. Preserve the existing
source-of-truth guidance and clarify that EAGLE3 entries are not unconditionally
validated.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9208e21d-d7bd-48bc-a02e-2b64f6ed7515
📒 Files selected for processing (3)
docs/source/deployment/3_unified_hf.rstexamples/hf_ptq/README.mdplugins/modelopt/skills/deployment/references/support-matrix.md
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/hf_ptq/README.md
- docs/source/deployment/3_unified_hf.rst
…port Review found that ✅ claimed more than the suite substantiates. Verified each point against the code: - test_deploy.py is marked `release` and collects only under --run-release, which no workflow passes, so the cases do not run on PR CI. Legend now says 'declared in the release deployment suite' and the section states both that limit and the load-and-generate scope. - Every backend sends the same plain-text COMMON_PROMPTS, so no image or audio reaches a processor. The VLM table is now labelled text-only smoke coverage. - The harness builds a speculative config only when the model ID contains 'eagle', so the Medusa case runs ordinary generation -> downgraded to ⚠. - Wan 2.2 runs the autoregressive text helper and asserts on text, never a video path -> downgraded to ⚠. - Kimi K2.6 Eagle3 is declared in test_kimi without the MODELOPT_LOCAL_EAGLE_MODEL gate and does cover vLLM; split from the gated rows and the blanket 'vLLM not covered' claim dropped. - Restored rows the rebuild silently dropped rather than downgraded: DeepSeek R1/V3 FP8 and Llama 4 Maverick NVFP4, both now ⚠. - Split Qwen 3 8B/14B from 32B, which has no FP8 entry. - Fixed a leftover 'every Y above' from the pre-review legend, and qualified the TRT-LLM v1.2.0 floor as the oldest version stated rather than the oldest that works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
4 participants
What does this PR do?
Type of change: documentation
Fixes NVBug 6550792 / OMNIML-5693.
The Unified HF Checkpoint Deployment Model Support Matrix listed 9 model families and no VLMs, while
tests/examples/hf_ptq/test_deploy.pydeclares deployment cases for ~80 checkpoints across TRT-LLM, vLLM, and SGLang — includingQwen2.5-VL,Qwen3-VL-235B, andNemotron-3-Nano-Omni. QA (the filer) could not use the doc to scope testing, and users could not tell what is actually covered.Filing also surfaced that the matrix lived in three places that had drifted apart: only the
.rstlisted Qwen3-VL, only the README listed Qwen3.5 MoE, and the skill reference had neither.Changes
Rebuilt the matrix in
docs/source/deployment/3_unified_hf.rstfromtest_deploy.py, split into language models, vision-language/multimodal, speculative decoding drafters, and diffusion.Stated plainly what the matrix is and is not. Review established that the original "CI-validated" framing claimed more than the suite substantiates, so a What this matrix is based on section now leads with two limits:
releaseand collects only under--run-release, which no workflow passes — these are declared cases, not PR-gated coverage.The legend follows from that: ✅ = declared in the suite, ⚠ = expected to work but not a suite entry (or an entry that does not exercise the feature the row names),
-= not in the suite. Sections that would otherwise over-read carry their own qualifiers — VLM rows are labelled text-only smoke coverage, and Medusa and Wan 2.2 are ⚠ with the reason stated.Removed the two duplicate copies, replacing them with links, so there is one table to maintain.
Fixed stale prose: the deployment tabs still claimed FP8-only support on vLLM v0.6.5 and a source build of SGLang main from Jan 2025, both contradicting the version table above them. The TRT-LLM floor moves to v1.2.0, qualified as the oldest version stated rather than the oldest that works.
Dropped the Phi series from the deployment matrix, following [NVBug: 6563509] Drop Phi-3-vision / Phi-4-multimodal PTQ support #2115 (NVBug 6563509) and confirmation that Phi-4 is being deprecated.
Usage
N/A — documentation only.
Testing
docutilsparse of the modified.rst: no warnings or errors from the new content; all 5 tables parse with every cell in the correct column.test_deploy.pyby AST-parsing theModelDeployerList(...)calls rather than by eye; the scope caveats were each verified againsttests/_test_utils/deploy_utils.py.pre-commit run --files …passes;build-docsgreen.Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Two known follow-ups, neither in scope here:
test_deploy.py. Consolidating to one copy removes the three-way drift but not the doc-vs-test drift; a generator plus a CI check would close it.For the filer (@kenny Kang): the ✅ cells are the scope the release deploy suite declares, and
test_deploy.pycarries the checkpoint, TP size, and minimum SM version per entry — but please read the legend first, since those cases are not currently executed by CI.