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. |
|
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. 📝 WalkthroughWalkthroughThe quantization recipe search skill now requires ModelOpt PTQ recipe guidance before candidate selection. It applies the guidance to quantization scope, KV-cache scheme, and calibration method, and requires citations or explanations for deviations. ChangesQuantization recipe guidance
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in the reviewed documentation workflow. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ## Recipe Guidance | ||
|
|
||
| Before selecting an initial or subsequent ModelOpt candidate, read |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2478 +/- ##
=======================================
Coverage 70.74% 70.74%
=======================================
Files 601 601
Lines 66300 66300
=======================================
Hits 46906 46906
Misses 19394 19394
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: Zhehao Hu <zhehaoh@nvidia.com>
c3dada5 to
2e3c80b
Compare
|
/claude review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| Before choosing a recipe, read `modelopt_recipes/ptq.md` in the source checkout | ||
| used for PTQ. Resolve all `modelopt_recipes/...` paths against that checkout. | ||
| If it is missing, follow | ||
| [Env-1. Get ModelOpt source](../common/environment-setup.md#env-1-get-modelopt-source). | ||
| Report if the guide cannot be read. |
There was a problem hiding this comment.
[SUGGESTION] The "source checkout" requirement is the right call — I confirmed pyproject.toml ships only modelopt_recipes = ["**/*.yml", "**/*.yaml"] as package data, so ptq.md genuinely does not exist in a wheel and a pip-installed user must follow Env-1. But that same asymmetry creates a silent skew this paragraph doesn't close.
What happens: for a pip-installed user, ptq.md comes from a freshly cloned checkout while the YAMLs also exist in the installed wheel. modelopt/recipe/loader.py:_resolve_recipe_path resolves a relative --recipe modelopt_recipes/... filesystem-first (cwd-relative), then falls back to BUILTIN_RECIPES_LIB. So if the PTQ run's cwd is not that checkout, the guide and YAML you reviewed come from the clone while the YAML that actually calibrates comes from the installed library — potentially a different revision.
Why it matters: the fallback is silent for this case. The FutureWarning at modelopt/recipe/loader.py:89 only fires for deprecated tier prefixes, not for a checkout-vs-built-in divergence, so nothing surfaces the mismatch. The cited rationale would describe a recipe that never ran.
Line 70's "ensure the quantization run uses the recipe and imported configs reviewed" states the requirement but names no mechanism. Naming one makes it executable:
| Before choosing a recipe, read `modelopt_recipes/ptq.md` in the source checkout | |
| used for PTQ. Resolve all `modelopt_recipes/...` paths against that checkout. | |
| If it is missing, follow | |
| [Env-1. Get ModelOpt source](../common/environment-setup.md#env-1-get-modelopt-source). | |
| Report if the guide cannot be read. | |
| used for PTQ. Resolve all `modelopt_recipes/...` paths against that checkout, and | |
| pass candidates to `ptq` as absolute paths under it (or run PTQ with that | |
| checkout as the working directory) so a relative path cannot fall back to the | |
| installed recipe library at a different revision. |
| Use the guide to choose quantization scope, KV-cache scheme, and calibration | ||
| method. Inspect the recipe YAML and its imported configs. When handing the | ||
| recipe to `ptq`, ensure the quantization run uses the recipe and imported configs | ||
| reviewed. Then perform the existing coverage, compatibility, and evaluation | ||
| checks. |
There was a problem hiding this comment.
[SUGGESTION] "Inspect the recipe YAML and its imported configs" + "ensure the quantization run uses the recipe and imported configs reviewed" is not achievable as literally written for the general-recipe path, because the skill you delegate to steers away from YAMLs there.
plugins/modelopt/skills/ptq/SKILL.md:71 says: "General PTQ recipes in modelopt_recipes/general/ptq/ correspond to the same formats — --qformat is the simpler way to use them." A --qformat nvfp4 run loads no recipe YAML at all, so whenever this guidance leads to a general scheme (the common case for a first candidate — ptq.md §Choosing a general recipe), the delegated run can legitimately never touch the file whose sections you just cited. The instruction then either blocks the documented ptq path or gets quietly ignored.
Model-specific recipes are unaffected — ptq/SKILL.md:56 already prefers --recipe <path> and already requires inspecting include/exclude patterns, which composes cleanly with this section.
Worth splitting the two cases explicitly, e.g.:
Use the guide to choose quantization scope, KV-cache scheme, and calibration
method. Inspect the recipe YAML and its imported configs. When handing a recipe
to `ptq`, keep the reviewed artifacts authoritative: pass `--recipe` so the run
loads exactly what you inspected. `--qformat` is acceptable only for a general
scheme whose `modelopt_recipes/general/ptq/` YAML was the basis of the review —
note that substitution in the portfolio table. Then perform the existing
coverage, compatibility, and evaluation checks.
|
Claude review — Scope: full review. 1 file changed (+22/−0), documentation-only: Findings — CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 2 Prior review findings are resolved. My earlier pass on
Verified as correct:
The two SUGGESTIONs — both fall out of the same asymmetry the PR creates, and neither blocks:
Risk: low. Documentation-only, additive, no runtime/checkpoint/public-API surface, backward compatible as the PR body states. The pin caveat for consumers tracking the plugin at a commit is correctly called out in Additional Information. Approving — the two SUGGESTIONs sharpen the cross-skill hand-off and do not block. |
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (claude-opus-5) — DM the bot to share feedback.
Approving: a 22-line, docs-only addition to quant-recipe-search/SKILL.md that is internally consistent — modelopt_recipes/ptq.md exists and does cover scope, KV-cache, and calibration, and the ../common/environment-setup.md#env-1-get-modelopt-source link and anchor both resolve.
No action needed.
2 participants
What does this PR do?
Type of change: documentation
Updates
quant-recipe-searchto require readingmodelopt_recipes/ptq.mdbefore selecting initial or subsequent quantization candidates.The skill uses the guide to inform quantization scope, KV-cache scheme, and calibration method. It requires inspecting candidate YAMLs and supporting configs, citing relevant guide sections, and explaining deviations. Existing coverage, runtime compatibility, and evaluation checks remain required.
Usage
Invoke
quant-recipe-searchas usual. The skill reads the guide from the ModelOpt source checkout.Testing
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
This change places the reading requirement in the shared recipe-search skill so consumers receive it directly. Consumers that pin the ModelOpt plugin to a commit must update their pin to adopt it.
Summary by CodeRabbit