Skip to content

Conversation

@petrex
Copy link
Collaborator

@petrex petrex commented Nov 16, 2025

TLDR: Optimize import time

Summary

  • Replace the 99k-line literal Hadamard matrix module with a lightweight loader (torchao/prototype/spinquant/_hadamard_matrices.py) that lazily deserializes tensors from _hadamard_matrices.pkl, keeping the public get_hadXX APIs unchanged while avoiding multi‑MB source files.
  • Ship the serialized matrices alongside the module by adding _hadamard_matrices.pkl (~345 KB) and updating setup.py so it’s included in package_data.
  • Preserve semantics by returning cloned torch.Tensor instances from the cached payload; import time is ~13× faster and git churn drops from 2 MB to 2 KB of Python plus the pickle.

Testing
python - <<'PY' ... (imports old vs new module with a stubbed torch)
Before: 75.9 ms import time
After: 5.7 ms import time

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 16, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 1af0bbd with merge base ba6f428 (image):
💚 Looks good so far! There are no failures yet. 💚

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 Nov 16, 2025
@petrex petrex added the topic: not user facing Use this tag if you don't want this PR to show up in release notes label Nov 16, 2025
@vkuzo
Copy link
Contributor

vkuzo commented Nov 17, 2025

The import time benefit is nice! I do have some questions about including an opaque .pkl file though, seems like a potential attack vector. Is there a way to generate this file dynamically during the setup process using an observable rule instead of generating it offline in a non-source-controlled way?

@petrex
Copy link
Collaborator Author

petrex commented Nov 18, 2025

The import time benefit is nice! I do have some questions about including an opaque .pkl file though, seems like a potential attack vector. Is there a way to generate this file dynamically during the setup process using an observable rule instead of generating it offline in a non-source-controlled way?

good point! just push the change to generate .pkl in setup.py (from json). thanks for the feedback

@petrex petrex requested a review from vkuzo November 18, 2025 16:29
@vkuzo
Copy link
Contributor

vkuzo commented Nov 18, 2025

thank you for doing this! two follow-up questions:

  1. would we get most of the import-time benefit if we just ship the json and avoid the opaque .pkl / additional setup.py complexity? any take on that?
  2. do we have confidence that numerics from using the hadamard transforms are still good

@petrex
Copy link
Collaborator Author

petrex commented Nov 19, 2025

thank you for doing this! two follow-up questions:

  1. would we get most of the import-time benefit if we just ship the json and avoid the opaque .pkl / additional setup.py complexity? any take on that?
  2. do we have confidence that numerics from using the hadamard transforms are still good
  1. Benchmarking shows that loading from JSON is only ~3ms slower than Pickle on first access, which is negligible.
  2. Is there any unit test for for this feature? All green in CI.

@vkuzo
Copy link
Contributor

vkuzo commented Nov 19, 2025

thank you! CI is green, so looks good.

@vkuzo vkuzo merged commit 7d5e2f6 into pytorch:main Nov 19, 2025
46 checks passed
@petrex petrex self-assigned this Nov 20, 2025
namgyu-youn pushed a commit to namgyu-youn/ao that referenced this pull request Nov 21, 2025
* Refactor Hadamard matrices loading

* lint

* Generate Hadamard pickle during setup

* lint
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.

2 participants