8000 Pin all root requirements to major versions by jondea · Pull Request #150833 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

Pin all root requirements to major versions #150833

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Pin all root requirements to major versions #150833

wants to merge 1 commit into from

Conversation

jondea
Copy link
Contributor
@jondea jondea commented Apr 8, 2025

Builds regularly fail due to major changes in build packages (most recently #150149), should we pin all the root requirements.txt to at least major version?

I made this a draft because I didn't really know the right solution, but

  • pyproject.toml has different build-requirements to requirements.txt? Which one is canonical? And should we have 2?
  • The manylinux CI builds seems to pip install -r requirements.txt but the Ubuntu unit testing CI uses pip install -r requirements-ci.txt. @malfet suggested here that we should pin build requirements in CI but not for local development, should we have another set of requirements for just manylinux?
  • Should the requirements be baked into the builder Docker images? At least then we can build with known good build dependencies by choosing a specific commit of the builder image (e.g. cpu-aarch64-af5c1b96e251422ad5fb05f98c1f0095f9c9d1cf). At the moment, the CI build scripts do pip install, but this could be done in the Dockerfile, it would have the added benefit of speeding up the CI and making the builds more reproducible.

@jondea
Pin all root requirements to major versions
02987a7 
Builds regularly fail due to major changes in build packages, most
recently pytorch#150149
@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Apr 8, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/150833

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 02987a7 with merge base cdf3b63 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Apr 8, 2025
@jondea
Copy link
Contributor Author
jondea commented Apr 8, 2025

@atalman, you may have some useful insight here

@jondea jondea marked this pull request as ready for review April 16, 2025 11:12
@mikaylagawarecki mikaylagawarecki requested a review from malfet April 16, 2025 15:03
@mikaylagawarecki mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 16, 2025
@malfet
Copy link
Contributor
malfet commented May 13, 2025
  • Should the requirements be baked into the builder Docker images?

Yes, they are already, see https://github.com/pytorch/pytorch/blob/main/.ci/docker/requirements-ci.txt

malfet
malfet requested changes May 13, 2025
View reviewed changes
Copy link
Contributor
@malfet malfet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind creating an issue first to outline why you think one is better than another?
My counter argument for pinning the dependencies in this file, is that we make it hard for developers who want to be on bleeding edge (say if someone wants to start building PyTorch against cpython-3.14, we should not cripple them by constraining upper version they can use.

But if you want reproducible builds, one can use requirements-ci.txt which pins dependencies specific to Python runtime version (for example some version of numpy are compatible with python-3.9 but incompatible with 3.13)

@jondea
Copy link
Contributor Author
jondea commented May 14, 2025

Thanks for the feedback. That makes sense, I agree the root requirements.txt is the wrong place to pin build requirements.

  • Should the requirements be baked into the builder Docker images?

Yes, they are already, see https://github.com/pytorch/pytorch/blo 8000 b/main/.ci/docker/requirements-ci.txt

The unit test CI installs .ci/docker/requirements-ci.txt into the Dockerfile

pytorch/.ci/docker/ubuntu/Dockerfile

Line 39 in 4b8b7c7

RUN bash ./install_conda.sh && rm install_conda.sh install_magma_conda.sh common_utils.sh /opt/conda/requirements-ci.txt /opt/conda/requirements-docs.txt

but the manywheel builds use the root requirements.txt

pytorch/.ci/manywheel/build_common.sh

Line 103 in 1d9401b

retry pip install -qr requirements.txt

pytorch/.ci/aarch64_linux/aarch64_ci_build.sh

Line 22 in 4b8b7c7

pip install -r /pytorch/requirements.txt

Should the manywheel builds also use .ci/docker/requirements-ci.txt? Or given that this includes some packages not necessary for building (e.g. numba), I think it may make sense to have a new .ci/docker/requirements-manywheel.txt or .ci/docker/requirements-build.txt with the necessary subset?

I'm happy for this PR to be rejected, and I can make a new one with a different approach, or I can make an issue to discuss further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malfet malfet malfet requested changes

@atalman atalman Awaiting requested review from atalman

Assignees
No one assigned
Labels
open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jondea @malfet @pytorchbot @mikaylagawarecki
0