Skip to content

Convert SmoothQuant test to unittest #2659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 6, 2025

Conversation

namgyu-youn
Copy link
Contributor

Fix part of #1621

Migrate test file (test/prototype/test_smoothquant.py) framework from pytest to unittest.

Copy link

pytorch-bot bot commented Aug 1, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/2659

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures, 1 Cancelled Job

As of commit 7217f8f with merge base 8d4a5d8 (image):

NEW FAILURES - The following jobs have failed:

CANCELLED JOB - The following job was cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 1, 2025
Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

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

Thanks!

@jerryzh168
Copy link
Contributor

@namgyu-youn if you are interested in contributing more, making smoothquant API similar to AWQ (#2400) is a good task

we are planning to graduate these two features to official support

@jerryzh168 jerryzh168 added the topic: not user facing Use this tag if you don't want this PR to show up in release notes label Aug 2, 2025
@namgyu-youn
Copy link
Contributor Author

@namgyu-youn if you are interested in contributing more, making smoothquant API similar to AWQ (#2400) is a good task

we are planning to graduate these two features to official support

@jerryzh168 Thanks for your suggestion. I will look into it before resolving more #1621 . Also, could you check #2644? It fixes part of #1621 (same as this PR).

cls.quant_mode_options = ["static", "dynamic"]
cls.devices = ["cpu"] + (["cuda"] if torch.cuda.is_available() else [])
cls.input_dtypes = (torch.float, torch.bfloat16, torch.half)
def _setup_quantized_model(self, model, alpha, quant_mode, calibration_data):
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it's probably fine to inline these as well, there are not many lines of code here

@namgyu-youn namgyu-youn requested a review from jerryzh168 August 2, 2025 05:05
"""Test the margin error of SmoothQuant across bias, alpha, dtype, etc."""
self._run_compute_accuracy_test(bias, alpha, quant_mode, device, input_dtype)

def _run_compute_accuracy_test(self, bias, alpha, quant_mode, device, input_dtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can also remove this function, seems no need to define a function here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, those functions are undefined for more brevity in 3ffdd10 .

f"device={device}, dtype={input_dtype}",
)

def _compute_reference_out(self, m_ref, data, alpha, quant_mode, bias, input_dtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's used only once, I'd suggest not to define new functions

"""Test save/load recipe functionality."""
self._run_save_load_recipe_test(alpha, quant_mode, device, input_dtype)

def _run_save_load_recipe_test(self, alpha, quant_mode, device, input_dtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

also this one

- Uncorrect API usage ( `common_utils`) is fixed
@namgyu-youn namgyu-youn requested a review from jerryzh168 August 2, 2025 10:02
"""Set up class-level configuration for tests."""
# Skip tests on ROCm (AMD GPU) due to compatibility issues
if torch.version.hip is not None:
raise unittest.SkipTest("Skipping the tests in ROCm")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does unittest.skip(...) work?

Copy link
Contributor Author

@namgyu-youn namgyu-youn Aug 5, 2025

Choose a reason for hiding this comment

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

@jerryzh168 https://docs.pytorch.org/docs/stable/notes/hip.html shows how torch.version.hip works. It is None in CUDA, str (ROCm version) in ROCm, and None in CPU-only. That is, this test is skipped only when ROCm available.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I mean using unittest.skip instead of raise the SkipTest error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, but there is no conditioner in the unittest.skip . How about using unittest.skipif? See https://docs.python.org/3/library/unittest.html#unittest.skipIf for the reference, and I like this change because the code can be more brevity

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sounds good, thanks

@namgyu-youn
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@jerryzh168 jerryzh168 merged commit 785f3dd into pytorch:main Aug 6, 2025
19 of 22 checks passed
@namgyu-youn namgyu-youn deleted the unittest_smoothquant branch August 7, 2025 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing Use this tag if you don't want this PR to show up in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants