Skip to content
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

Graceful handling of cpp extensions #296

Merged
merged 8 commits into from
May 30, 2024
Merged

Graceful handling of cpp extensions #296

merged 8 commits into from
May 30, 2024

Conversation

msaroufim
Copy link
Member

@msaroufim msaroufim commented May 30, 2024

This fixes issues from #288

More specifically

  1. We introduce a new env variable so we can install ao locally without building cpp extensions USE_CPP=0 pip install . this is useful out of convenience for faster iteration cycles
  2. If the extensions were built but failed for whatever reason on a host device then importing those extensions will be skipped and ao will not crash. This also means you can import torchao from inside torchao/
  3. If cpp extensions aren't build then the fp6 and ops tests are skipped

Copy link

pytorch-bot bot commented May 30, 2024

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit ac44a52 with merge base 4c1d568 (image):
💚 Looks good so far! There are no failures yet. 💚

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 May 30, 2024
test/test_ops.py Outdated Show resolved Hide resolved
torchao/dtypes/float6_e3m2.py Outdated Show resolved Hide resolved
@msaroufim
Copy link
Member Author

Failure in CI seems unrelated so review would still be helpful

README.md Outdated
Copy link
Collaborator

@vayuda vayuda May 30, 2024

Choose a reason for hiding this comment

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

On wsl2:
USE_CPP=0 pip install -e. runs into https://pastebin.com/CMhYTn20
but USE_CPP=0 python setup.py develop works

Copy link
Collaborator

Choose a reason for hiding this comment

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

pip installing wheel fixed this issue. Maybe add this to dev-requirements.txt

Copy link
Collaborator

@gau-nernst gau-nernst 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. Still not sure why some people still face problems. Without a reproducible way to trigger the problems, it is hard to see what would solve them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what the standard should be, but I know other tests (ex. quantization) import some boolean like "TORCH_VERSION_AFTER_2_4" and use that instead of a try except block

Copy link
Contributor

@jerryzh168 jerryzh168 May 30, 2024

Choose a reason for hiding this comment

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

I think the main problem is we are not sure why/when the import fails, so the boolean probably won't help here..

@msaroufim msaroufim requested review from vayuda and jerryzh168 May 30, 2024 20:13
@msaroufim msaroufim merged commit e7837d7 into main May 30, 2024
13 checks passed
@msaroufim msaroufim deleted the nocpp branch May 30, 2024 22:04
dbyoung18 pushed a commit to dbyoung18/ao that referenced this pull request Jul 31, 2024
* Graceful handling of cpp extensions

* update

* push

* yolo

* revert some changes'

* Update __init__.py

* Update README.md

---------
yanbing-j pushed a commit to yanbing-j/ao that referenced this pull request Dec 9, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants