Skip to content

FouroverSix - #2882

Closed
krishnateja95 wants to merge 10 commits into
vllm-project:mainfrom
krishnateja95:FourOverSix
Closed

FouroverSix#2882
krishnateja95 wants to merge 10 commits into
vllm-project:mainfrom
krishnateja95:FourOverSix

Conversation

@krishnateja95

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the generate_gparam call in src/llmcompressor/observers/base.py to include the four_over_six parameter retrieved from self.args. The reviewer suggested enhancing robustness by also checking self.args.observer_kwargs for this parameter, providing a clear code suggestion to implement this fallback.

Comment on lines 100 to 104
global_scale = generate_gparam(
-global_absmax.reshape(1), global_absmax.reshape(1)
-global_absmax.reshape(1),
global_absmax.reshape(1),
four_over_six=getattr(self.args, "four_over_six", False),
)

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.

medium

The four_over_six parameter might be configured as an observer-specific argument and stored within self.args.observer_kwargs rather than directly as an attribute on self.args (which is a QuantizationArgs instance). To ensure robustness, we should check both self.args attributes and self.args.observer_kwargs.

Suggested change
global_scale = generate_gparam(
-global_absmax.reshape(1), global_absmax.reshape(1)
-global_absmax.reshape(1),
global_absmax.reshape(1),
four_over_six=getattr(self.args, "four_over_six", False),
)
global_scale = generate_gparam(
-global_absmax.reshape(1),
global_absmax.reshape(1),
four_over_six=getattr(self.args, "four_over_six", False)
or getattr(self.args, "observer_kwargs", {}).get("four_over_six", False),
)

@mergify mergify Bot added the two-reviews When a PR requires two reviews label Jun 30, 2026
@mergify

mergify Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 2 of 2 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require one maintainer review 👀 reviews
🔴 Require two reviews 👀 reviews

🔴 Require one maintainer review

Waiting for any of

  • approved-reviews-by=HDCharles
  • approved-reviews-by=brian-dellabetta
  • approved-reviews-by=dsikka
  • approved-reviews-by=kylesayrs
  • approved-reviews-by=yiliu30
This rule is failing.

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

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

🔴 Require two reviews

Waiting for

  • #approved-reviews-by >= 2
This rule is failing.

PRs labelled "two-reviews" must have at least two approving reviews before merging.

  • #approved-reviews-by >= 2
  • #changes-requested-reviews-by = 0

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto 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: db1f21da-0091-4085-91d1-6acf06ae7ee6

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

Walkthrough

A single cosmetic reformatting in Observer.get_qparams: the two global_absmax.reshape(1) arguments passed to generate_gparam are split onto separate lines. No logic, inputs, outputs, or control flow are changed.

Changes

Observer Formatting

Layer / File(s) Summary
Reformat generate_gparam arguments
src/llmcompressor/observers/base.py
Splits -global_absmax.reshape(1) and global_absmax.reshape(1) onto separate lines in the generate_gparam call; no functional change.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested labels

refactor

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is a non-descriptive phrase that does not indicate the actual change. Use a concise, specific title that mentions the main change, such as the formatting-only update in Observer.get_qparams.
Description check ❓ Inconclusive No description was provided, so the PR intent cannot be determined from the text. Add a brief description of what changed and why, even if the update is only formatting.
✅ Passed checks (3 passed)
Check name Status Explanation
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

Warning

Review ran into problems

🔥 Problems

Linked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped vllm-project/compressed-tensors.


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 added the Refactor Code cleanup and/or improvements to existing features label Jun 30, 2026
@github-actions

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

@kylesayrs kylesayrs added the ready When a PR is ready for full CI testing before merge label Jul 8, 2026
@mergify

mergify Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The quality checks have failed. Please run make style and make quality under
the root directory to adddress the lint failures. You will need to install the
dev optional install to get the required linting packages:
https://github.com/vllm-project/llm-compressor/blob/main/CONTRIBUTING.md

@mergify

mergify Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @krishnateja95.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Jul 10, 2026
Accident push to this remote, reverted

This reverts commit 72572be.
@kylesayrs

Copy link
Copy Markdown
Collaborator

Closing for #3015

@kylesayrs kylesayrs closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase ready When a PR is ready for full CI testing before merge Refactor Code cleanup and/or improvements to existing features two-reviews When a PR requires two reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants