Skip to content

add imatrix calibration data resolution to quant config - #787

Open
Roderick-Wu wants to merge 2 commits into
mainfrom
Roderick-Wu/imatrix-fix
Open

add imatrix calibration data resolution to quant config#787
Roderick-Wu wants to merge 2 commits into
mainfrom
Roderick-Wu/imatrix-fix

Conversation

@Roderick-Wu

Copy link
Copy Markdown
Collaborator

Signed-off-by: Roderick Wu <Roderick-Wu@h100-01.nemg-001.lab.rdu2.dc.redhat.com>
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7c77315e-5adf-4a65-a797-3dbef6cdbd9f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

Changes

Calibration Detection

Layer / File(s) Summary
Weight observer calibration check
src/compressed_tensors/quantization/quant_config.py
requires_calibration_data now returns True when a scheme has a weight observer whose lowercase value is imatrix_mse.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: kylesayrs, brian-dellabetta

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: adding imatrix-related calibration handling in quant config.
Description check ✅ Passed The description is only a link, but it points to the related upstream PR for this change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch Roderick-Wu/imatrix-fix

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/compressed_tensors/quantization/quant_config.py`:
- Around line 300-305: Replace the boolean-wrapped any() condition in the
surrounding scheme loop with a direct if condition checking scheme.weights,
scheme.weights.observer, and its case-insensitive "imatrix_mse" value; preserve
the existing return True behavior when all checks match.
🪄 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: Pro

Run ID: 582515c3-a4f2-4fe9-8063-19b202b49815

📥 Commits

Reviewing files that changed from the base of the PR and between bf81005 and 5875776.

📒 Files selected for processing (1)
  • src/compressed_tensors/quantization/quant_config.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Comment on lines +300 to +305
if any(
scheme.weights is not None
and scheme.weights.observer is not None
and scheme.weights.observer.lower() == "imatrix_mse"
):
return True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Replace any() with a direct condition

any() is receiving a boolean, not an iterable, so every call reaching this branch raises TypeError: 'bool' object is not iterable. The surrounding loop already evaluates one scheme at a time; use a direct if condition instead.

Proposed fix
-            if any(
+            if (
                 scheme.weights is not None
                 and scheme.weights.observer is not None
                 and scheme.weights.observer.lower() == "imatrix_mse"
             ):
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if any(
scheme.weights is not None
and scheme.weights.observer is not None
and scheme.weights.observer.lower() == "imatrix_mse"
):
return True
if (
scheme.weights is not None
and scheme.weights.observer is not None
and scheme.weights.observer.lower() == "imatrix_mse"
):
return True
🤖 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 `@src/compressed_tensors/quantization/quant_config.py` around lines 300 - 305,
Replace the boolean-wrapped any() condition in the surrounding scheme loop with
a direct if condition checking scheme.weights, scheme.weights.observer, and its
case-insensitive "imatrix_mse" value; preserve the existing return True behavior
when all checks match.

Source: Path instructions

Signed-off-by: Roderick Wu <Roderick-Wu@h100-01.nemg-001.lab.rdu2.dc.redhat.com>
if (
scheme.weights is not None
and scheme.weights.observer is not None
and scheme.weights.observer.lower() == "imatrix_mse"

@HDCharles HDCharles Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets make a list of observers that require calibration in llm-compressor and use it here instead of hardcoding a bunch of observer names

@mergify

mergify Bot commented Aug 17, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require one maintainer review 👀 reviews

🔴 Require one maintainer review

Waiting for

  • #changes-requested-reviews-by = 0
  • any of:
    • approved-reviews-by=HDCharles
    • approved-reviews-by=brian-dellabetta
    • approved-reviews-by=dsikka
    • approved-reviews-by=kylesayrs
This rule is failing.

All PRs must have at least one approving review from a maintainer before merging.

  • #changes-requested-reviews-by = 0
  • any of:
    • approved-reviews-by=HDCharles
    • approved-reviews-by=brian-dellabetta
    • approved-reviews-by=dsikka
    • approved-reviews-by=kylesayrs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants