Skip to content

pip / pip_tools requirements update target upgrading by default #1334

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

Closed
chrislovecnm opened this issue Jul 21, 2023 · 8 comments
Closed

pip / pip_tools requirements update target upgrading by default #1334

chrislovecnm opened this issue Jul 21, 2023 · 8 comments
Labels
Can Close? Will close in 30 days if there is no new activity

Comments

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Jul 21, 2023

I'm testing on the main branch with the bzlmod example.

We have two issues:

  1. The Python script is not updating the requirements file in Linux - dependency_resolver.py
  2. bazel throws errors when you have an empty file requirement for Python 3.9. If you cp /dev/null to the requirement file:
    examples/bzlmod/requirements_lock_3_9.txt

I am testing on WSL/Linux with bazelisk.

When running the target

https://github.com/bazelbuild/rules_python/blob/93f5ea2f01ce7eb870d3ad3943eda5d354cdaac5/examples/bzlmod/BUILD.bazel#L18

bazelisk run. //:requirements_3_9.update

The above command should update the file:

https://github.com/bazelbuild/rules_python/blob/93f5ea2f01ce7eb870d3ad3943eda5d354cdaac5/examples/bzlmod/BUILD.bazel#L21

But the contents of the file are not changing.

On Linux, this line of code is run:

https://github.com/bazelbuild/rules_python/blob/93f5ea2f01ce7eb870d3ad3943eda5d354cdaac5/python/pip_install/tools/dependency_resolver/dependency_resolver.py#L179

But the following lines do not run. The cli() call is not throwing an error, but the script seems to exit.

The run command outputs the updated requirements file changes, but the file is not updated.

When you run the same file with Windows Powershell, the Windows requirements file is updated correctly.

@chrislovecnm
Copy link
Contributor Author

#1123

The above PR did a bunch of work implementing this functionality. We have unit tests here:

https://github.com/bazelbuild/rules_python/tree/main/tests/compile_pip_requirements

But they are not running using bzlmod.

@chrislovecnm
Copy link
Contributor Author

chrislovecnm commented Jul 21, 2023

A workaround I have used it delete the file and run the command. This works with this target:

https://github.com/bazelbuild/rules_python/blob/93f5ea2f01ce7eb870d3ad3943eda5d354cdaac5/examples/bzlmod/BUILD.bazel#L28

But with the 3.9 target, we get a weird error about the toolchain. I need to include that error here as well.

TODO - include the error message here.

@rickeylev
Copy link
Collaborator

Not sure offhand. Is this the code that uses an atexit callback to copy the modified file to where it's supposed to go? ... Ah yes, it is. Maybe that has something to do with it?

@chrislovecnm
Copy link
Contributor Author

chrislovecnm commented Jul 24, 2023

So this is odd. Python 3.10 piptools is finding yamllint==1.32.0, but for Python 3.9, piptools is finding yamllint==1.28.0. Under Python 3.9, none of the dependencies are updated, but under Python 3.10, they are. The same thing is happening on Windows and Linux.

You must update the requirements to set the yamllint>=1.32.0; it then works as expected.

@chrislovecnm chrislovecnm changed the title requirements update target not working on LInux (WSL) requirements update target not working with Python 3.9 Jul 24, 2023
@chrislovecnm chrislovecnm changed the title requirements update target not working with Python 3.9 pip / pip_tools requirements update target upgrading by default Jul 24, 2023
@chrislovecnm
Copy link
Contributor Author

I have found the problem or feature.

dependency_resolver.py does not pass in --upgrade to pip_tools compile. See:

https://github.com/jazzband/pip-tools/blob/7ccf975ebeedc3ae5bd840f21493eb5bdcdcf230/piptools/scripts/compile.py#L183C1-L189C2

We would need to add the following:

sys.argv.append("--upgrade")

To somewhere around here:

https://github.com/bazelbuild/rules_python/blob/23354a9b607ff0207e9a3c0cbe16724ec53997c1/python/pip_install/tools/dependency_resolver/dependency_resolver.py#L163C20-L163C20

The question is should we upgrade the package requirements by default? A user can do the following:

bazel run :requirements_3_9.update -- --upgrade

Which will give pip_tools the option to upgrade the packages to the highest version. This needs to be documented.

@groodt
Copy link
Collaborator

groodt commented Jul 24, 2023

I don't think anyone would want to upgrade their entire dependency closure every time. There probably should always be a choice. Or I guess somebody can add an .upgrade target?

I think the distinction between update and upgrade is clear enough, but only once one is aware of the differences in semantics. So it probably could do with some improved docs.

Semantic differences explained.

update - This is used generally when somebody is updating a single package in requirements.in and doesn't want to force a full resolve or upgrade of the entire closure aka requirements.txt. It takes the requirements.in and uses the requirements.txt as constraints to perform a minimal update to the changed closure. This runs quite fast in general. It also has a nice property that it is stable and won't drift when new packages are published, this makes it usable in tests. The semantics of the test effectively confirm that: "the input dependencies in requirements.in are fulfilled by the resolved set of dependencies in requirements.txt".

upgrade - This is used to fully re-resolve the dependency closure from requirements.in. This will ignore any constraints or the contents of the existing requirements.txt file. This will run more slowly because it will start from a clean slate and download / build packages as required. It's not a "stable" operation, because everytime it runs, it is impacted by external state of packages published to pypi. This is a big-bang upgrade. Which is often a risky move when there are a large number of dependencies: more difficult diff to read, greater risk of error due to unrelated upgrades.

Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Jan 21, 2024
Copy link

This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? Will close in 30 days if there is no new activity
Projects
None yet
Development

No branches or pull requests

3 participants