Skip to content

Modernized packaging metadata #143

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agronholm
Copy link

@agronholm agronholm commented Mar 23, 2025

This adds a standard pyproject.toml, allowing the package to be installed according to the instructions in README (which previously required undocumented steps like installing packaging and torch beforehand and using the --no-build-isolation flag. This flag is now documented, and build requirements have been specified. This PR also allows you to build an sdist for distribution on the PyPI by bypassing the GPU detection when building metadata only or an sdist.

@woct0rdho
Copy link

I just managed to build it in the build isolation, which is required for reproducible build and distributing wheels to others. You can see what I've done at https://github.com/woct0rdho/SageAttention

pybind11 is also required when building it. It's tricky to specify both torch (with index URL at download.pytorch.org ) and pybind11 (not in that index URL) in the build-time dependencies, and the easiest way I could think of is to use simpleindex

@agronholm
Copy link
Author

agronholm commented Mar 24, 2025

I don't know how you did it, as when I try the same, I just get: RuntimeError: Cannot find a CUDA installation. CUDA must be available to build the package.

@agronholm
Copy link
Author

Adding pybind11 to build dependencies does not help.

@agronholm
Copy link
Author

Besides, don't you need to point to a torch version specific to your installed CUDA version?

@woct0rdho
Copy link

woct0rdho commented Mar 24, 2025

setup.py imports CUDA_HOME from torch, so you must install torch+the correct CUDA version in the build isolation

(I guess it's also possible to modify setup.py so it can build with torch+cpu downloaded from PyPI, then we don't need two index URLs. But anyway, if we want to build with nightly torch, then we still need two index URLs)

@agronholm
Copy link
Author

setup.py imports CUDA_HOME from torch, so you must install torch+the correct CUDA version in the build isolation

Yes, that's exactly what I just said, right?

@woct0rdho
Copy link

Yes exactly

@agronholm
Copy link
Author

agronholm commented Mar 24, 2025

Yeah, so this is why I left torch out of the build dependencies and instead opted to instruct users to install it beforehand and then disable build isolation when installing sageattention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants