-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add build-system.requirements to pyproject.toml #493
Conversation
We tried this earlier (v1.0.4 to 1.0.8 i think) and it was a lot of headache because the torch requirement conflicts with different ways that people install pytorch in their system. |
Ah good to know, thanks. I was just about to build some wheels to test this but sounds like its not going to be worth it. FWIW, the way I'm doing development is:
I noticed that as long as the |
There are more details here: pypa/pip#8437 One option, and the simplest, may be to remove the |
[build-system] | ||
requires = [ | ||
"torch", |
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.
Another idea.. maybe we just remove torch here?
That way, for most people, they're going to run pip install flash-attn
and it will work correctly instead of complaining about packaging not found. We don't need torch for them, because its essentially just going to find and install the binary wheel.
Then in the cuda build codepath, we could search for torch and if its not there, then we print out an error telling the user to try again with --no-build-isolation
.
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.
Here's the defaults used when pyproject.toml doesn't have entries:
https://github.com/pypa/pip/blob/main/src/pip/_internal/pyproject.py#L125-L127
build_system = {
"requires": ["setuptools>=40.8.0", "wheel"],
"build-backend": "setuptools.build_meta:__legacy__",
so it pulls in wheel and setuptools, but not packaging. And since the setup.py depends on packaging, people see that error about packaging not being found.
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 tried both (with / without torch in the requires). Somehow each approach works for 1 group of users but breaks for another group of users. I don't quite understand how, but it seems the pytorch setup is very different across users (some install with pip / conda, some install from source, some uses nvidia docker files).
Now that we have wheels, the situation might be different as you said. Even if we're downloading the wheel, we need to know the pytorch version to download the correct wheel.
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.
Even if we're downloading the wheel, we need to know the pytorch version to download the correct wheel.
Oh yea damn, good point. What a mess.
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 see, I didn't know there's a default for when pyproject.toml doesn't have the entries. The torch requirement is the main headache.
Okay I think the only option is to remove the pyproject.toml, so this project is treated as a legacy setup.py project. That way you don't need to worry about all this nonsense, and users don't have to deal with the strange situation now where Here's the code in https://github.com/pypa/pip/blob/main/src/pip/_internal/pyproject.py#L101-L103 if has_pyproject and not has_setup:
use_pep517 = True
elif build_system and "build-backend" in build_system:
use_pep517 = True
elif use_pep517 is None:
use_pep517 = (
has_pyproject
or not importlib.util.find_spec("setuptools")
or not importlib.util.find_spec("wheel")
) |
lol i just wanted to use pyproject.yaml to set black options. This is surprisingly hard :D |
lol yea, pretty insane side-effect of adding a linting configuration 🤣 |
What if we put |
That could work. I'm not sure what your workflow is, do you run black cli directly or is it integrated into your editor environment? Let me check if black supports reading configuration from subdirectories. |
According to psf/black#1370 (comment)
So if you plan to |
yeah let's do that |
i've pushed |
should fix #453