Skip to content

[Quark] Support online block-diagonal rotations in dense GEMM layers - #32272

Open
fxmarty-amd wants to merge 6 commits into
vllm-project:mainfrom
fxmarty-amd:online-rotation-dense-standalone
Open

fxmarty-amd wants to merge 6 commits into
vllm-project:mainfrom
fxmarty-amd:online-rotation-dense-standalone

Conversation

@fxmarty-amd

@fxmarty-amd fxmarty-amd commented Jan 13, 2026

Copy link
Copy Markdown
Contributor

This PR adds support to load quantized models making use of online block-diagonal rotations in dense GEMM layers (OCP MX, FP8, INT8) and produced through AMD Quark library, based on the config.json's quantization_config indicating which layers are using online rotations.

See the notes: https://quark.docs.amd.com/latest/release_note.html#support-for-online-rotations-online-hadamard-transform

This is a naive non-optimized implementation.

We'll likely release models in the future. For the moment, we simply use in the CI:

Motivation

Online rotation (hadamard/orthogonal) has been shown to be a promising technique to recover from the accuracy drop seen in low-precision formats as MXFP4.

Using block-diagonal hadamard or orthogonal transforms is a popular strategy recently, as per e.g.

Test plan

pytest tests/quantization/test_quark.py -s -vvvvv -k "test_online_transform_loading"

@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 adds support for online block-diagonal rotations in OCP MX dense layers, a new feature for Quark quantization. The changes are primarily within vllm/model_executor/layers/quantization/quark/schemes/quark_ocp_mx.py to handle the configuration for these rotations, create and process the rotation matrix weights, and apply the transformation during the forward pass. The overall logic appears correct and aligns with the feature description. However, I've identified several instances of unsafe dictionary access that could lead to KeyError exceptions if the quantization configuration is not structured as expected. I've provided specific suggestions to improve the robustness of this code by using the .get() method for safer dictionary key access.

Comment thread vllm/model_executor/layers/quantization/quark/schemes/quark_ocp_mx.py Outdated
Comment thread vllm/model_executor/layers/quantization/quark/schemes/quark_ocp_mx.py Outdated
@mergify

mergify Bot commented Jan 21, 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, @fxmarty-amd.

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

Signed-off-by: Felix Marty <Felix.Marty@amd.com>
@fxmarty-amd
fxmarty-amd force-pushed the online-rotation-dense-standalone branch from 31b751f to 1f2345a Compare January 21, 2026 15:45
@fxmarty-amd fxmarty-amd changed the title [Quark] Support online block-diagonal rotations in OCP MX dense layers [Quark] Support online block-diagonal rotations in dense GEMM layers Jan 21, 2026
@fxmarty-amd
fxmarty-amd marked this pull request as ready for review January 21, 2026 16:20
@fxmarty-amd
fxmarty-amd requested a review from tjtanaa as a code owner January 21, 2026 16:20

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Comment thread vllm/model_executor/layers/quantization/quark/transform.py Outdated
@fxmarty-amd

Copy link
Copy Markdown
Contributor Author

cc @BowenBao @mgoin @robertgshaw2-redhat could you have a look? Thank you!

@BowenBao BowenBao 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.

LGTM w/ comment

Comment on lines +178 to +179
quant_config: dict[str, Any],
layer_names: list[str],

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.

nit: I feel this increases the scope of information passed in, where previously weight_quant_spec and input_quant_spec are scoped in on this particular layer. now QuarkScheme also needs to handle parsing info from global quant_config based on layer_names. Do you think there's a way to reduce the surface?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let me have a look shortly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you have an other suggestion? We could pass to each QuarkScheme instead directly:

      use_online_rotation: bool = False,
      rotation_config: dict[str, Any] | None = None,
      rotation_size: int | None = None,

but I don't feel like it is better

Comment thread tests/quantization/test_quark.py
@BowenBao

BowenBao commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

@tjtanaa could you review again if this can be landed? thanks!

@mergify

mergify Bot commented Feb 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, @fxmarty-amd.

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 Feb 10, 2026
@mergify mergify Bot removed the needs-rebase label Mar 24, 2026
Signed-off-by: Felix Marty <Felix.Marty@amd.com>
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you!

@github-actions github-actions Bot added the stale Over 90 days of inactivity label Jul 22, 2026
@fxmarty-amd

Copy link
Copy Markdown
Contributor Author

not stale

@github-actions github-actions Bot added unstale Recieved activity after being labelled stale and removed stale Over 90 days of inactivity labels Jul 24, 2026
@mergify mergify Bot added the quantization label Jul 24, 2026
@mergify

mergify Bot commented Jul 24, 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, @fxmarty-amd.

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 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase quantization unstale Recieved activity after being labelled stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants