adding vllm unit tests#1517
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GPU vLLM fakequant tests: CI matrix and nox session for containerized GPU tests, DeepSeek V3 test model helpers, pytest conftest to enable vLLM RPC serialization, and end-to-end quantization tests with fixtures asserting quantizer coverage and static _amax invariants. ChangesvLLM Fakequant Test Suite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1517 +/- ##
==========================================
- Coverage 76.63% 76.50% -0.14%
==========================================
Files 476 476
Lines 51813 52208 +395
==========================================
+ Hits 39707 39940 +233
- Misses 12106 12268 +162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kinjalpatel27
force-pushed
the
kinjal/vllm_fq_unit
branch
from
7b708cb to
0ebeee6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/gpu_vllm_fakequant/torch/quantization/test_vllm_dynamic_modules.py`:
- Around line 16-17: Move the test suite file
tests/gpu_vllm_fakequant/torch/quantization/test_vllm_dynamic_modules.py into
one of the approved GPU test folders (e.g., tests/gpu or the appropriate
tests/gpu_* target) so it complies with the repository test-directory policy,
then update any CI and nox configuration entries that reference the old path
(search for occurrences of "gpu_vllm_fakequant" or the filename
"test_vllm_dynamic_modules.py" in noxfiles and workflow YAMLs) to point to the
new location and ensure the test is included in the correct GPU test collection.
- Around line 352-353: Replace the broad "except Exception as exc" that
unconditionally calls pytest.skip with a targeted check that only skips for
known environment/capability failures and re-raises all other exceptions: catch
specific exception types (e.g., ImportError, OSError, RuntimeError) or inspect
exc's message for known indicators (CUDA/vLLM model download failures) and call
pytest.skip only in those matched cases; for any other exc, re-raise the
exception so real regressions in the vLLM/model loading/quantization path
surface. Ensure this logic surrounds the same try block that currently ends with
pytest.skip and continues to use pytest.skip(...) for the allowed cases.
🪄 Autofix (Beta)
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: f28e2a71-b5a2-446b-a077-041bd9668aed
📒 Files selected for processing (4)
.github/workflows/gpu_tests.ymlnoxfile.pytests/gpu_vllm_fakequant/conftest.pytests/gpu_vllm_fakequant/torch/quantization/test_vllm_dynamic_modules.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/gpu_tests.yml:
- Around line 55-56: The vLLM image tag in the workflow (the line containing
"container_image: docker.io/vllm/vllm-openai:v0.21.0") is out of sync with
examples/vllm_serve/Dockerfile which uses v0.10.2; update the workflow entry to
use the same tag as the Dockerfile (change the container_image value to
docker.io/vllm/vllm-openai:v0.10.2) so the comment "Keep in sync with
examples/vllm_serve/Dockerfile" is honored, or conversely update the Dockerfile
to v0.21.0 if you intend to standardize on the newer release—ensure both places
reference the identical image tag.
- Line 56: The workflow pins the OpenAI-server image to container_image:
docker.io/vllm/vllm-openai:v0.21.0 which may be vulnerable to
GHSA-3mwp-wvh9-7528; verify whether that v0.21.0 release includes the patch and
if not either update the tag to the patched vLLM image (reference the patched
release tag) or add mitigations: enforce input validation and a max "n"
parameter and/or enable rate-limiting in the vLLM server invocation;
specifically, locate the container_image: docker.io/vllm/vllm-openai:v0.21.0
entry and either change it to the patched image tag or add environment
variables/command-line args or ingress rate-limiting configuration to bound the
`n` parameter and throttle requests.
🪄 Autofix (Beta)
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: c96aa3eb-b472-440a-91c2-bf1b5151e78b
📒 Files selected for processing (3)
.github/workflows/gpu_tests.ymlnoxfile.pytests/gpu_vllm_fakequant/torch/quantization/test_vllm_dynamic_modules.py
🚧 Files skipped from review as they are similar to previous changes (2)
- noxfile.py
- tests/gpu_vllm_fakequant/torch/quantization/test_vllm_dynamic_modules.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gpu_tests.yml (1)
25-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand the PR gate to cover the new vLLM lane’s actual inputs.
Right now
any_changedwill stay false for changes intests/_test_utils/**orexamples/vllm_serve/Dockerfile, even though this lane now depends on the shared tiny-model builders and the workflow explicitly says the image pin must stay in sync with that Dockerfile. That lets helper regressions or image drift skipgpu_vllmentirely.Suggested patch
files: | .github/workflows/gpu_tests.yml + examples/vllm_serve/Dockerfile modelopt/** noxfile.py pyproject.toml + tests/_test_utils/** tests/gpu/** tests/gpu_megatron/** tests/gpu_trtllm/** tests/gpu_vllm/**🤖 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 @.github/workflows/gpu_tests.yml around lines 25 - 33, The PR gate for the gpu_vllm lane is missing inputs so any_changed stays false for changes to shared tiny-model builders and the vLLM Dockerfile; update the files list in .github/workflows/gpu_tests.yml (the block that populates any_changed) to include tests/_test_utils/** and examples/vllm_serve/Dockerfile (and any other tiny-model builder paths if separate) so changes to those files will trigger the gpu_vllm lane and keep the image pin in sync with the Dockerfile referenced by the gpu_vllm job.
🤖 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 `@tests/gpu_vllm/torch/quantization/test_vllm_dynamic_modules.py`:
- Around line 353-355: The test unconditionally asserts VllmMLAAttention exists
but that symbol can be None on builds without MLA; update the
test_registry_has_mla_attention function to first check if VllmMLAAttention is
None and call pytest.skip("MLA not available") (or return) when absent,
otherwise perform the existing assertions against QuantModuleRegistry; reference
the VllmMLAAttention symbol and the QuantModuleRegistry in the change.
---
Outside diff comments:
In @.github/workflows/gpu_tests.yml:
- Around line 25-33: The PR gate for the gpu_vllm lane is missing inputs so
any_changed stays false for changes to shared tiny-model builders and the vLLM
Dockerfile; update the files list in .github/workflows/gpu_tests.yml (the block
that populates any_changed) to include tests/_test_utils/** and
examples/vllm_serve/Dockerfile (and any other tiny-model builder paths if
separate) so changes to those files will trigger the gpu_vllm lane and keep the
image pin in sync with the Dockerfile referenced by the gpu_vllm job.
🪄 Autofix (Beta)
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: 94d4592d-e642-4fab-aa1f-b84f904dafaa
📒 Files selected for processing (5)
.github/workflows/gpu_tests.ymlnoxfile.pytests/_test_utils/torch/transformers_models.pytests/gpu_vllm/conftest.pytests/gpu_vllm/torch/quantization/test_vllm_dynamic_modules.py
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
kinjalpatel27
force-pushed
the
kinjal/vllm_fq_unit
branch
from
6bd7465 to
597ecca
Compare
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| # Static-amax invariant: every enabled quantizer must own an ``_amax`` | ||
| # after calibration. ``kv_b_proj`` is exempt — vLLM's MLA decode path | ||
| # reads its weight directly and never calls its forward. |
There was a problem hiding this comment.
[SUGGESTION] The kv_b_proj not in name exemption is broader than the stated rationale.
The comment says vLLM's MLA decode path reads kv_b_proj's weight directly and never calls its forward. That justifies exempting input_quantizer._amax (no observed activations), but weight_quantizer._amax is computed from the weight tensor at calibration time independently of any forward call, so it should still be present. As written, the substring match also exempts the output_quantizer and any future quantizer attached to a module whose name contains kv_b_proj.
If a future regression silently strips weight calibration for kv_b_proj, this assertion would not catch it. Consider narrowing to the specific quantizer that's expected to be missing, e.g.:
if not hasattr(module, "_amax") and not name.endswith("kv_b_proj.input_quantizer"):
quantizers_without_amax.append(name)(or whichever exact suffix matches the bypassed quantizer in the MLA path).
There was a problem hiding this comment.
Claude review passed — no blocking issues found.
Verified the new tests against the vLLM plugin: imported symbols (_VLLMParallelLinear, _QuantFusedMoEBase, VllmMLAAttention, _ATTENTION_TYPES, disable_compilation) exist in modelopt/torch/quantization/plugins/vllm.py, and the class-name strings asserted in the parallel-linear counts (QuantRowParallelLinear, QuantQKVParallelLinear, QuantMergedColumnParallelLinear) match what _DMRegistryCls._get_dynamic_class_name generates (prefix("Quant") + nn_cls.__name__). The MoE isinstance check correctly covers both _QuantVLLMFusedMoE and _QuantVLLMSharedFusedMoE via the shared _QuantFusedMoEBase. VLLM_ALLOW_INSECURE_SERIALIZATION is set in conftest before any vLLM import, which is the right place. The workflow's python→python3 and explicit container_image paths are sane.
Findings (all SUGGESTION, non-blocking):
- 2 inline comments —
kv_b_projexemption is broader than its stated rationale;python3vspythoninconsistency in noxfile.
Risk: low. Pure test-only change, no production code touched.
LGTM
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Test-only PR adding tests/gpu_vllm/ with end-to-end fakequant coverage for _VLLMParallelLinear, _QuantFusedMoEBase, vLLM Attention, and MLAAttention across TinyLlama / TinyQwen3-MoE / TinyDeepseekV3. Approach (driving mtq.quantize inside the worker via LLM.collective_rpc, with _dummy_run(1) as the calibration forward loop) looks reasonable and the assertions are meaningful (per-class linear counts, per-quantizer slot presence, and _amax registration with a documented kv_b_proj exemption). Couple of points worth a human eye:
- Dockerfile/noxfile pin mismatch.
noxfile.pyand.github/workflows/gpu_tests.ymlboth pindocker.io/vllm/vllm-openai:v0.20.0with the comment "Keep in sync with examples/vllm_serve/Dockerfile", butexamples/vllm_serve/Dockerfileis onvllm/vllm-openai:v0.10.2. Either the Dockerfile needs bumping in this PR or the comment/pin is wrong — please reconcile before merge. v0.20.0image tag. Worth double-checking the tag actually exists on Docker Hub (vLLM's mainline at the time of writing is ~0.10/0.11). If it's a pre-release or internal tag, the gpu_vllm CI job will fail on first run.- Subprocess coverage disabled for gpu_vllm. Reasonable workaround given the documented engine-core IPC deadlock, but it does mean the new test surface contributes zero coverage to Codecov; the gpu-tests job still uploads the (empty-ish) coverage file. Just flagging; not blocking.
No design-review concerns — this is test infra, not a new abstraction. Standard NVIDIA license headers (exception applies). Nudging rather than approving so an owner can confirm the Docker tag/pin question.
|
|
||
| import gc | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Bot comment.
Top-level from vllm import LLM here means pytest collection imports vLLM before conftest.py has had a chance to set VLLM_ALLOW_INSECURE_SERIALIZATION — which works only because pytest loads conftests before test modules in the same package. Worth a brief note in the conftest docstring confirming that ordering, since the comment currently warns "must precede any import vllm" without explaining why the test-module import is OK.
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 (1)
examples/vllm_serve/Dockerfile (1)
1-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the image as a non-root user before
CMD.The Dockerfile never sets
USER, so workloads run as root by default. That weakens container isolation and matches the DS-0002 security finding.🔐 Suggested hardening patch
FROM vllm/vllm-openai:v0.20.0 @@ # Allow users to run without root RUN chmod -R 777 /workspace + +# Drop root privileges for runtime +RUN useradd --create-home --uid 10001 --shell /bin/bash appuser && \ + chown -R appuser:appuser /workspace +USER appuser # Override the ENTRYPOINT from the base image to allow flexible usage ENTRYPOINT []🤖 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 `@examples/vllm_serve/Dockerfile` around lines 1 - 44, The Dockerfile currently leaves processes running as root (no USER set) which weakens isolation; create a non-root user (e.g., "appuser") after installing packages and preparing files, chown the WORKDIR (/workspace) and Model-Optimizer files to that user (update or replace the chmod -R 777 /workspace step with chown -R appuser:appuser /workspace and Model-Optimizer ownership), then add USER appuser before the ENTRYPOINT/CMD lines so the container runs as that non-root user; keep root for earlier steps (apt-get, pip installs, extensions precompile) and only switch to the non-root account once image setup is complete.
🤖 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 `@examples/vllm_serve/Dockerfile`:
- Around line 1-44: The Dockerfile currently leaves processes running as root
(no USER set) which weakens isolation; create a non-root user (e.g., "appuser")
after installing packages and preparing files, chown the WORKDIR (/workspace)
and Model-Optimizer files to that user (update or replace the chmod -R 777
/workspace step with chown -R appuser:appuser /workspace and Model-Optimizer
ownership), then add USER appuser before the ENTRYPOINT/CMD lines so the
container runs as that non-root user; keep root for earlier steps (apt-get, pip
installs, extensions precompile) and only switch to the non-root account once
image setup is complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c9f75c4-ebe9-4c34-a2b4-4d201f036ef5
📒 Files selected for processing (2)
examples/vllm_serve/Dockerfiletests/gpu_vllm/torch/quantization/test_vllm_dynamic_modules.py
| @@ -1,4 +1,4 @@ | |||
| FROM vllm/vllm-openai:v0.10.2 | |||
| FROM vllm/vllm-openai:v0.20.0 | |||
There was a problem hiding this comment.
From previous benchmarking, I know 0.20 has an accuracy bug on kimi k2.6. Not sure if it's fixed. 0.19.0 looks safer to me.
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
3 participants
What does this PR do?
Type of change: new tests
This PR adds unit tests for vLLM fakequant, specifically testing code in
modelopt/torch/quantization/plugins/vllm.pyTesting
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: N/AAdditional Information
Summary by CodeRabbit
Tests
Chores
Documentation