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.

Sign up for GitHub

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

Jump to bottom

add build-system.requirements to pyproject.toml #493

Closed
wants to merge 1 commit into from
Closed

add build-system.requirements to pyproject.toml #493

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
[tool.black]
line-length = 100
target-version = ['py38']
target-version = ['py38']

[build-system]
requires = [
"torch",
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

"einops",
"packaging",
"ninja",
"wheel",
"setuptools"
]