-
Notifications
You must be signed in to change notification settings - Fork 705
chore: Build and store bdist wheels #181
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Thanks! I will merge it tomorrow. Closing #179 now. |
Hi @ko3n1g, may I know why there is a |
BTW, is it OK to release Linux only and CUDA 12 only? We don't support other platform and CUDA 11. |
And |
I did some updates and lints on your branch (not tested), you can continue to work on this. Many thanks! |
Hey @LyricZhao, sorry for the absence, I was on vacation and just returned yesterday. Picking this now up again.
Does this mean we don't need to build torch/cuda specific wheels? In that case, I can simplify the whole logic significantly. I wasn't too familiar with the src code and assumed it needed platform specific wheels like FA or TE. |
I guess we should have torch-specific wheel and cuda-major version (12/13) wheels. |
Sorry for my late reply, too busy recently. |
bffc64b
to
bd86ac7
Compare
Signed-off-by: oliver könig <[email protected]>
Thanks for the fixes and linting @LyricZhao ! The I noticed the last commit accidentally (?) removed some args from the cuda-extension and re-added it. Please let me know if this is good! I think this PR should otherwise be good to go |
Sorry, I don't get it. The current version failed to compile the CUDA extension. Actually, this is not CUDA extension, but just the JIT CPP part. And in my opinion, this should be always built. So do you agree if I remove the |
Another thing is about I want to remove this as well. If git is available, the downloaded wheel (for the cached codepath) must be aligned with the local version (commit ID included and not local uncommited changes). Do you agree with this? |
Signed-off-by: oliver könig <[email protected]>
Signed-off-by: oliver könig <[email protected]>
Yes of course, please feel free to refactor as you see it best for DG! To lay out my thoughts and motivation:
I hope this makes the workflow clear:) This is the standard procedure we're doing at FlashAttention, mamba, groupedgemm and others. But again, I'm not familiar with the specifics of DG, so if you believe a different approach fits DG better, please feel free to refactor and push to this branch and merge it:) |
Thanks! I get it, I will refactor & test it tomorrow and merge! Now env values seems default for CI but not for users (so some errors occur). |
Local copy of #179