Fix vLLM compilation guard for models without marker - #2518
kinjalpatel27 wants to merge 3 commits into
Conversation
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/Model-Optimizer/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthrough
ChangesvLLM compilation handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete issue remains that should prevent merging after normal checks. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/claude review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review summary
Scope: full review — 2 changed files (35 additions / 10 deletions): modelopt/torch/quantization/plugins/vllm.py and tests/gpu_vllm/torch/quantization/test_vllm_dynamic_modules.py. Both reviewed in full, plus the three disable_compilation call sites (examples/vllm_serve/fakequant_worker.py, examples/vllm_serve/sparse_attn_worker.py, the existing gpu_vllm tests).
Findings — CRITICAL: 0, IMPORTANT: 1, SUGGESTION: 0
Most impactful finding
plugins/vllm.py:249-258 — [IMPORTANT Compatibility] the save/restore mechanics of this change are correct (exception-safe, nesting-safe, and the tests cover both wrapper layouts and both marker states), but the premise that a vLLM model may legitimately lack do_not_compile doesn't hold: vLLM's support_torch_compile decorator sets that attribute unconditionally in __init__ and is the only reader of it. An object without the marker is therefore not the compile-wrapped module, so installing the marker on it does not disable compilation — it only silences the AttributeError that was telling you the target was wrong.
The risk is a silent no-op where there used to be a loud failure: mtq.quantize then runs with torch.compile active, able to recompile while quantizers are being inserted. The GLM-5.3 run in the description used eager mode, where compilation is off anyway, so it does not distinguish "guard works" from "guard is a no-op". It also makes the language_model branch permanently unreachable (and now silently so) for wrappers that expose both model and language_model.
Suggested direction, in the inline comment: resolve the module that actually owns do_not_compile by searching model / model.model / model.language_model.model, and fall back to installing the marker only with a warnings.warn so the no-op case stays visible. Two minor robustness points are folded in there as well — use "do_not_compile" in vars(target) rather than hasattr so a class-level default isn't permanently shadowed by an instance attribute, and vars(target).pop(..., None) rather than bare del so cleanup can't raise inside finally and mask an exception from the body.
A quick way to confirm or refute on the GLM-5.3 checkpoint: print type(model).__name__, type(model.model).__name__, and which objects along that chain have do_not_compile. If one of them does, the fix is to target it; if none does, the model isn't compile-wrapped and the warning path is the honest behavior.
Risk assessment
Low-to-moderate. Small, well-tested, backward-compatible change that strictly widens what disable_compilation accepts, and the restore semantics are an improvement over the previous version. The concern is not regression for existing models but whether the guard actually guards for the new case it was added for.
Nothing to flag on mode/state composability, export, or performance — the change touches neither modelopt_state nor any export path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2518 +/- ##
==========================================
+ Coverage 68.78% 78.36% +9.57%
==========================================
Files 603 605 +2
Lines 66796 68396 +1600
==========================================
+ Hits 45947 53596 +7649
+ Misses 20849 14800 -6049
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:
|
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-6-astra) — DM the bot to share feedback.
Approve: the guard handles missing markers and restores instance or inherited state correctly, with focused regression coverage and no unrelated changes.
No action needed.
Signed-off-by: Kinjal Patel <kinjalpravin@nvidia.com>
3444797 to
76cf969
Compare
This branch has not been deployed
2 participants
What does this PR do?
Type of change: Bug fix
Makes the vLLM
disable_compilationcontext manager support inner model implementations that do not predefine ado_not_compileattribute, including GLM-5.3. The context manager now installs the marker temporarily and removes it afterward, while preserving and restoring existing marker values for other vLLM models.Adds regression coverage for both supported wrapper layouts:
model.modelandmodel.language_model.model.Usage
No caller changes are required.
Testing
tests/gpu_vllm/torch/quantization/test_vllm_dynamic_modules.py: 24 passed with vLLM 0.28.vllm/vllm-openai:glm53-flashon OCI-JHB and served the GLM-5.3-Flash BF16 checkpoint withQUANT_CFG=NVFP4_DEFAULT_CFG, TP=4, eager mode, and BF16 KV cache.do_not_compilefailure point, inserted 1,700 quantizers, enabled 456 weight quantizers, reached a healthy API server, and returned a relevant manual prompt response.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
Validated against GLM-5.3-Flash using ModelOpt commit
869b64fcee0b20be323663449b00e8c52940a289.Summary by CodeRabbit
Bug Fixes
Tests