Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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.
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.