Skip to content

Conversation

brian-dellabetta
Copy link
Collaborator

@brian-dellabetta brian-dellabetta commented Sep 29, 2025

SUMMARY:
This PR

  • Removes pile-val-dataset from e2e tests, as it is no longer used in examples and the processing logic was flawed
  • Fixes a model validation error introduced in [Multi-modifier] Support scoped application of quantization config/status #1772 that was preventing AWQModifier from running one of the validations, causing it to be in an invalid state (AWQModifier.validate_model_after was preventing QuantizationMixin.validate_model_after from running). With these changes, tests pass and the compressed model generates meaningful responses. It was previously generating all 0s

TEST PLAN:
CADENCE=nightly TEST_DATA_FILE=tests/e2e/vLLM/configs/w4a16_grouped_quant_sym_awq.yaml pytest -s tests/e2e/vLLM/test_vllm.py and
CADENCE=nightly TEST_DATA_FILE=tests/e2e/vLLM/configs/w4a16_grouped_quant_asym_awq.yaml pytest -s tests/e2e/vLLM/test_vllm.py
both pass with output like

PROMPT:
The capital of France is
GENERATED TEXT:
 Paris, which is also the country's largest city.

PROMPT:
The president of the US is
GENERATED TEXT:
 named, but the name of the Vice President is not given. In the case

PROMPT:
My name is
GENERATED TEXT:
 Emily and I am from Canada. I have always been fascinated with

Copy link

👋 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.

@brian-dellabetta brian-dellabetta changed the title remove pile-val backup from tests [tests] remove pile-val-backup dataset from tests Sep 29, 2025
@brian-dellabetta brian-dellabetta marked this pull request as ready for review September 29, 2025 20:53
@brian-dellabetta brian-dellabetta added the ready When a PR is ready for review label Sep 29, 2025
fynnsu
fynnsu previously approved these changes Sep 29, 2025
Copy link
Collaborator

@fynnsu fynnsu left a comment

Choose a reason for hiding this comment

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

Looks good!

My only comment is maybe we should change the name of
tests/e2e/vLLM/recipes/WNA16/recipe_w4a16_group_quant_asym_awq.yaml
to match
tests/e2e/vLLM/recipes/WNA16/recipe_w4a16_awq_sym.yaml if we're just directly using the W4A16_ASYM scheme. But also fine as is.

shanjiaz
shanjiaz previously approved these changes Sep 29, 2025
Copy link
Collaborator

@shanjiaz shanjiaz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for fixing!

Signed-off-by: Brian Dellabetta <[email protected]>
@brian-dellabetta brian-dellabetta dismissed stale reviews from shanjiaz and fynnsu via 2dffddd September 29, 2025 22:04
@brian-dellabetta brian-dellabetta force-pushed the bdellabe/vllm-awq-test-fix branch from 64a8ea9 to 2dffddd Compare September 29, 2025 22:04
Signed-off-by: Brian Dellabetta <[email protected]>
Signed-off-by: Brian Dellabetta <[email protected]>
@brian-dellabetta brian-dellabetta merged commit 4c8c0a7 into main Sep 30, 2025
8 checks passed
@brian-dellabetta brian-dellabetta deleted the bdellabe/vllm-awq-test-fix branch September 30, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants