-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Fix incorrect installation instructions (for issue #37476) #37640
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
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
README.md
Outdated
| ## Installation | ||
|
|
||
| Transformers works with Python 3.9+ [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. | ||
| Transformers works with Python 3.9-3.12 [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. |
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.
The meaning behind the DRY principle is a bit deeper than "don't repeat yourself." Rather, DRY is a principle of software development aimed at reducing repetition of information likely to change. By documenting the versions in two places (README.md, requirements.txt file) the information has been repeated, which will inevitably lead to one or the other going stale.
Instead, keep the requirements inside of requirements.txt file and direct the reader to read that file, because it captures the same information.
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.
The meaning behind the DRY principle is a bit deeper than "don't repeat yourself." Rather, DRY is a principle of software development aimed at reducing repetition of information likely to change. By documenting the versions in two places (README.md, requirements.txt file) the information has been repeated, which will inevitably lead to one or the other going stale.
Instead, keep the requirements inside of requirements.txt file and direct the reader to read that file, because it captures the same information.
Great suggestion! Will do that.
README.md
Outdated
| ## Installation | ||
|
|
||
| Transformers works with Python 3.9+ [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. | ||
| Transformers works with Python 3.9-3.12 [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. |
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.
Nit: Python 3.9 to 3.12 or Python 3.9 - 3.12
Note the space in the second, which will distinguish from potentially oddball versions (i.e., there is no version with the literal expression 3.9-3.12).
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.
Nit:
Python 3.9 to 3.12orPython 3.9 - 3.12Note the space in the second, which will distinguish from potentially oddball versions (i.e., there is no version with the literal expression
3.9-3.12).
Thanks, will modify this!
requirements/install.sh
Outdated
|
|
||
| # === Install Required Packages === | ||
| echo "Installing required packages..." | ||
| pip install "jax>=0.4.1,<=0.4.13" |
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.
Why are these pip instructions different than the files listed inside the requirements.txt file? And why are the versions repeated all over the place? They're listed like 5 times, a quintuplication of information.
Is there a way to reference the requirements.txt file from the script?
Like xargs pip install < requirements.txt?
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.
Why are these pip instructions different than the files listed inside the
requirements.txtfile? And why are the versions repeated all over the place? They're listed like 5 times, a quintuplication of information.Is there a way to reference the
requirements.txtfile from the script?Like
xargs pip install < requirements.txt?
Referring to requirements.txt sounds more reasonable and I will do that, but I need to think about what to put in requirements.txt a bit more. Will ping you again if my revision is completed!
requirements/requirements.txt
Outdated
| @@ -0,0 +1,10 @@ | |||
| filelock | |||
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 don't recall having to install most of these on my system. Did you spin up a base Arch (or Debian) image with a bare minimum install and see what's required to run?
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 don't recall having to install most of these on my system. Did you spin up a base Arch (or Debian) image with a bare minimum install and see what's required to run?
Dear @DarkTyger, thank you so much for your detailed suggestions! I will try my best and keep polishing my PR until it can be merged.
As for requirements.txt, I was referring to setup.py and I believe the packages listed here are the prerequisites for installing the transformer packages. So I guess they should be included in the requirements.txt?
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.
Dear @DarkTyger, after some further investigation of the issue, I think I may need some clarifications on what the fix should be like. Considering your suggestions in the issue:
- fix python version requirements;
- a requirements.txt file that people can download;
- a shell script that can install and validate the requisite versions;
- explicitly list all instructions that'll work for most people.
The first one is very reasonable and I will submit a fix soon.
However, I'm not quite sure if adding requirements.txt, installation script, or explicitly listing the instructions is necessary? I'm asking this question for 2 reasons:
- setup.py may be sufficient for installation, and adding requirements.txt or installation script can be repetitive. I don't know what you mean by "work for most people"? It turns out using
pip install "transformers[all]"orpip install .[all]installs all the required packages and the quickstart examples can be used immediately after running one of these 2 commands. Therefore, adding requirements.txt or installation script can be repetitive, violating the DRY principle you mentioned earlier. - In the examples folder, requirements files are provided for different use cases, but they are quite different from task to task so it's hard to determine what are the core packages to include. Moreover, the examples and their requirements are not maintained frequently. Maybe the repo is not designed to use requirements.txt for installation.
As for the packages you mentioned (torch, tensorflow, flax, accelerate, and transformers), I believe pip install transformers[all] is the command you want to try. You may refer to here.
Please correct me if I made anything wrong.
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'm not sure how else to put this; the documentation is incorrect.
- Create a new virtual OS (or container) with latest Python.
- Visit https://pypi.org/project/transformers
- Follow the instructions as shown.
Expected
Instructions work.
Actual
pip install transformers fails (for a few reasons, including Python version and missing [all]).
Whether a script, a requirements.txt file, or an update to the documentation is the solution, wasn't really my point. My points are:
- when drafting instructions, avoid repeating information that is likely to change; and
- ensure the instructions are tested in a clean environment on supported platforms.
If following the instructions verbatim (and that's key) fails, then the instructions are wrong.
Aside, written instructions in README.md are hard to test automatically, which is why I recommended writing a script. If the instructions become "1. download script; 2. run script" then that can be tested within containerized environments. When problems are encountered, the script gets the fixes, and the automated test guards against regressions. This means the README.md needn't change to put in a fix because the requirements are captured programmatically.
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.
Hi @DarkTyger sorry for the late reply. I just made a new version of the PR, and would be grateful if you could check if this version looks good to you.
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've looked over the changes briefly. They look much better!
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.
Glad to hear that! Thank you for your suggestions!
@Rocketknight1 @ArthurZucker Could you please help check if this PR looks ok and consistent with the style of the repo?
Rocketknight1
left a comment
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.
Hi @Zephyr271828, I'm sorry for being late with this one, but I don't think we can accept this PR as-is, but we could accept some of the changes from it! I should have said something earlier, but there are a few issues:
- We don't want to limit our Python version compatibility to remain compatible with Tensorflow. Right now our TF/JAX support is mostly in maintenance mode, and PyTorch is the primary framework of the library!
- We don't want to add a separate
install.shscript, especially since we don't need the Py<3.13 version check!
However, some of the changes here are useful, and we could keep them! I'll flag them in the comments.
README.md
Outdated
| ## Installation | ||
|
|
||
| Transformers works with Python 3.9+ [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. | ||
| Transformers works with Python 3.9 - 3.12 [PyTorch](https://pytorch.org/get-started/locally/) 2.1+, [TensorFlow](https://www.tensorflow.org/install/pip) 2.6+, and [Flax](https://flax.readthedocs.io/en/latest/) 0.4.1+. |
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.
Keep it at 3.9+!
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.
Keep it at 3.9+!
It breaks with 3.13, therefore 3.9+ is incorrect.
| pip install transformers | ||
| pip install "transformers[torch]" | ||
|
|
||
| # uv | ||
| uv pip install transformers | ||
| uv pip install "transformers[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.
This change is good, we can keep it!
README.md
Outdated
| pip install . | ||
| pip install .[torch] | ||
| # optionally, you can use our script to verify python version, create venv, install from source, and verify installation. | ||
| bash scripts/install.sh "[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.
We can keep .[torch] but remove the reference to install.sh!
| "Programming Language :: Python :: 3.11", | ||
| "Programming Language :: Python :: 3.12", |
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.
These changes are good, and we could add 3.13 as well!
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.
3.13 as well!
It breaks on Arch Linux with Python 3.13 due to transitive dependencies being pinned to 3.12.
setup.py
Outdated
| ] | ||
| }, | ||
| python_requires=">=3.9.0", | ||
| python_requires=">=3.9,<3.13", |
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 don't want to limit to <3.13
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.
limit to <3.13
Have you tried it with Python 3.13? It breaks.
Aside, this is another example of a DRY violation. Why is the upper bound needed in so many places? If it isn't needed here because the version dependency is satisfied/documented elsewhere, then this can go.
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.
Hi @DarkTyger, like I said, TF support is in maintenance mode and PyTorch is the primary framework right now! We're not going to cap the Python version for the whole library to maintain TF compatibility.
Hi Matt @Rocketknight1 these suggestions totally make sense to me! I have submitted a new version of PR and would be happy if you could check if it's ready for merge:) |
Rocketknight1
left a comment
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.
LGTM now!
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…huggingface#37640) * debugging issue 36758 * debugging issue 36758 * debugging issue 36758 * updated attn_mask type specification in _flash_attention_forward * removed pdb * added a blank line * removed indentation * update constants * remove unnecessary files * created installation script, modified README * modified requirements and install.sh * undo irrelevant changes * removed blank line * fixing installation guide * modified README, python requirements, and install script * removed tests_otuput * modified README * discarded installation script and python<3.13 requirement
What does this PR do?
This PR is a proposed solution for issue #37476. Specifically, this PR modifies description in README.md, adds a requirements.txt file and an install.sh file:
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.