Skip to content

Add sparsity to benchmarking #1917

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 27, 2025
Merged

Add sparsity to benchmarking #1917

merged 9 commits into from
Mar 27, 2025

Conversation

jainapurva
Copy link
Contributor

@jainapurva jainapurva commented Mar 18, 2025 •
edited
Loading

Add sparsity support for benchmarking. The following support has been added

  • Sparsity Techniques, with Sparsify_
  • Sparsity + Quantize
  • Support filter functions, will need to be added later

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Mar 18, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit ef4cf36 with merge base 09c2760 (image):

NEW FAILURE - The following job has failed:

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

@facebook-github-bot facebook-github-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 Mar 18, 2025
@jainapurva jainapurva added topic: new feature Use this tag if this PR adds a new feature topic: for developers Use this tag if this PR is mainly developer facing topic: performance Use this tag if this PR improves the performance of a feature and removed topic: for developers Use this tag if this PR is mainly developer facing labels Mar 24, 2025
@jainapurva jainapurva marked this pull request as ready for review March 24, 2025 22:46
jcaip
jcaip reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@jcaip jcaip left a comment

Choose a reason for hiding this comment

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

thanks for working on this! just a couple questions but otherwise looks good

benchmarks/microbenchmarks/benchmark_inference.py Outdated
@@ -44,11 +45,33 @@ def run(config: BenchmarkConfig) -> BenchmarkResult:

# Use quantize_ to apply each quantization function to the model
m_copy = deepcopy(base_model).eval().to(config.device)
quantization_config = string_to_config(
config.quantization, high_precision_dtype=config.high_precision_dtype
aoBaseConfig = string_to_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

probably camel_case is better here?

benchmarks/microbenchmarks/test/benchmark_config.yml Outdated
@@ -1,9 +1,13 @@
# Sample configuration for inference benchmarks
benchmark_mode: "inference"
quantization_config_recipe_names:
- "baseline"
# - "baseline" Will always run a baseline instatance
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be commented out?

Copy link
Contributor Author

@jainapurva jainapurva Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We're running baseline case as default for any benchmarking param. the reason I listed it here as a comment is because I wanted to let users know that this will always run. Maybe I can simply add it to readme, and write the comment like
# Will run a baseline inference for model by default, without quantization for comparison

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: yeah I think that's better, I would just make it clear that it's not some commented out code.

benchmarks/microbenchmarks/test/benchmark_config.yml Outdated
- "int4wo-128"
- "marlin"
sparsity_config_recipe_names:
# - "none" Will always run a without sparsity instance
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Same as above

benchmarks/microbenchmarks/test/test_benchmark_inference.py Outdated
# Mock string_to_config to return valid configs
from torchao.quantization import Int4WeightOnlyConfig
from torchao.sparsity.sparse_api import (
BlockSparseWeightConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need BlockSparseWeightConfig here - should be semi-structured sparsity no?

benchmarks/microbenchmarks/test/test_benchmark_inference.py
self.assertIsInstance(result, BenchmarkResult)
self.assertTrue(hasattr(result, "model_inference_time_in_ms"))

# Test with block sparsity
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 see - can we split this into two tests then, one for int4+2:4 marlin, and one for block sparsity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jainapurva jainapurva requested a review from jcaip March 25, 2025 21:02
jainapurva and others added 6 commits March 25, 2025 15:28
@jainapurva
Add sparsity support in benchmarking
c49d512 
ghstack-source-id: c9e900d
ghstack-comment-id: 2752688588
Pull Request resolved: #1955
@jainapurva
Sparsity tests
407ea1d 
ghstack-source-id: ac08cd2
ghstack-comment-id: 2752688653
Pull Request resolved: #1956
@jainapurva
Add sparsify_ to benchmarking
65d4065 
ghstack-source-id: 4cf77f8
ghstack-comment-id: 2752688734
Pull Request resolved: #1957
@jainapurva
Support sparsify_ run
cea30b1 
ghstack-source-id: 8b459e2
ghstack-comment-id: 2752688801
Pull Request resolved: #1958
@jainapurva
Review updates
ae70fec 
ghstack-source-id: ded0179
ghstack-comment-id: 2752688871
Pull Request resolved: #1959
@jainapurva
Add profile support
d71baa3 
ghstack-source-id: df82466
ghstack-comment-id: 2752688926
Pull Request resolved: #1960
jcaip
jcaip reviewed Mar 26, 2025
View reviewed changes
benchmarks/microbenchmarks/test/test_utils.py Outdated
@@ -139,6 +191,7 @@ def test_generate_results_csv(self):
BenchmarkResult(
BenchmarkConfig(
quantization="int8wo",
sparsity="None",
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: why string None and not just None here?

@jainapurva jainapurva merged commit 3766ed7 into main Mar 27, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcaip jcaip jcaip approved these changes

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

@HDCharles HDCharles Awaiting requested review from HDCharles

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: new feature Use this tag if this PR adds a new feature topic: performance Use this tag if this PR improves the performance of a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jainapurva @jcaip @facebook-github-bot