-
Notifications
You must be signed in to change notification settings - Fork 617
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
Build Pip Wheels #523
Build Pip Wheels #523
Conversation
|
Oh btw we would love to have official wheels in the repo, if you want to contribute a PR :) |
@danthe3rd Thanks for following up after my mistake! I can gladly re-open this MR, it was intended as part of the discussion in #473 (comment). However, there is a lot of stuff that is still unclear to me, maybe you or anyone else on the team can give me more input on this, just so I can know what to focus on:
Those are some of the questions that I have, the rest is just a consequence of the above. Thank you! have a nice weekend. |
Let me get back to you next week. @fmassa @blefaudeux and @bottler might have some opinion as well |
pulling in @patrickvonplaten and @patil-suraj also, who were interested in helping or looking at that wheel building procedure at least |
Not personally in charge anymore, but trying to help out still :)
I'm actually amazed that this works out on GHA, I didn´t think that was possible. I'm guessing that it will quickly count against the Meta credits, but it could be tied to a release (vs. for all PRs) and that would be probably good enough ?
ahah, totally understood !
This can be automated indeed I believe, as you mentioned. I actually think that fairseq already has the flows for that
Not a real expert also, but should not be a blocker I think, these days it's trivial to change python versions (and most of them should be ABI compatible but I'm not 100% sure there)
Not sure either but same as above, should not be a blocker I think, let's not make perfect the enemy of great :)
as far as I'm concerned anything old can get the door if anything new is better 👯
Thank you, I didn´t know that this was up but it's great work, could be super duper useful to many people |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the code in the PR looks good for a v1 - we can take care of the upload+windows later. Just added a few comments before we can merge it
.github/workflows/wheels.yml
Outdated
- name: Install nvcc | ||
run: > | ||
wget https://developer.download.nvidia.com/compute/cuda/repos/debian11/x86_64/cuda-keyring_1.0-1_all.deb && | ||
dpkg -i cuda-keyring_1.0-1_all.deb && | ||
apt-get update && | ||
apt-get install cuda-nvcc-11-8 cuda-libraries-dev-11-8 -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should install the version corresponding to matrix.cuda
. There are some available github-action for that to help apparently: https://github.com/Jimver/cuda-toolkit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to have to try this first.
Currently, the build happens in a debian slim container, which should (at least in theory) work with this action. (I am still unsure)
PyTorch logs a warning to the console if the nvcc version is not the same as the one it was compiled with, but because it is only a minor version, pytorch logs that "this is likely not a problem" so I just winged it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up not using this action since it only works on debian but the container I am using is centos
Hi @AbdBarho! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
.github/workflows/wheels.yml
Outdated
build_wheels: | ||
strategy: | ||
matrix: | ||
# python: ["3.7", "3.8", "3.9", "3.10"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is pip currently not available for 3.7, 3.8, 3.9?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite the opposite, it is possible to compile to all of the above versions, this was just a test
Hey @AbdBarho, It's extremely cool that you're working on making pip wheels easily available for the community! I think many people use xformers fast attention mechanism and would love an easy way of installing it via pip. As maintainers of the diffusers library: https://github.com/huggingface/diffusers we'd love to more prominently feature xformers for training and would be very happy to help you! We're also in contact with the PyTorch team (gently pinging @ngimel just FYI). Please let us know if you're stuck with anything (github actions etc...) => we're happy to help! |
@patrickvonplaten Thank you very much! I am going to try to fit in this in my schedule tomorrow, I will report back if I face any problems. |
This PR is now ready for review. You can see the workflow and download the generated wheels here: I ask you all please to test the built wheels in your applications, I did a test with python3.10, torch1.12.1, and cuda 11.6 on my SD docker, and it worked without a problem, the more tests the better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great thank you! The CI failing is not due to this PR, so I'll merge.
Thanks a lot for your contribution!
@patrickvonplaten if you want to improve this workload, feel free to open a new PR.
In list of remaining things to do, I think we need to worry about windows as well (and as an extension, it would be great to have a CI for windows as well on all pull requests).
We don't plan to contribute to it in the short term, the only thing we will do is adding upload to pypi
@AbdBarho
I have tested 3.10.6, 3.7.12 on GCE with Deep Learning VM and 3.7.15 at Google Colab. @danthe3rd
I tried to find info on xformers repository.
Then I found this commit (#527) by @danthe3rd , and tried to fix like this,
But still it didn't work. Then, I found a info on Python 3.8's what's new page.
It's a matter of return statement. So, this
should be written like this in Python 3.7.
Here is a point of the difference between Python 3.7 and 3.8+.
Thanks! |
Are pip wheels working now that this PR is merged? |
What does this PR do?
Adds Github Workflow to build pip wheels. Fixes #473.
Before submitting
CUTCLASS
not found #473PR review
Anyone in the community is free to review the PR once the tests have passed.