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.

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

Upgrade pip to 21.3.1 #30927

Merged
merged 6 commits into from
Jan 21, 2022
Merged

Upgrade pip to 21.3.1 #30927

merged 6 commits into from
Jan 21, 2022

Conversation

dannyroberts
Copy link
Member

Product Description

No product change

Technical Summary

I was having issues locally with previous version, so I thought I'd update pip for everyone.

Safety Assurance

Safety story

We update pip regularly and it's never been an issue. If it were an issue it would be during setup of the virtualenv, and it would fail during deploy before switching over.

Automated test coverage

Tests check that the files edited here are internally consistent

QA Plan

None

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@dannyroberts dannyroberts added the product/invisible Change has no end-user visible impact label Jan 11, 2022
@dimagimon dimagimon added the dependencies Pull requests that update a dependency file label Jan 11, 2022
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

Why didn't the one in test-requirements.txt get updated?

pip==21.2.4
# via pip-tools

I am also surprised that pip is not in requirements.txt or docs-requirements.txt, but don't think it matters if it is not.

@millerdev
Copy link
Contributor

Does this new version of pip work with the version of pip-tools we require (6.2.0) or should that be upgraded too? It's not working for me:

$ pip --version
pip 21.3.1 from .../.pyenv/versions/3.9.7/envs/hq/lib/python3.9/site-packages/pip (python 3.9)

$ pip show pip-tools
Name: pip-tools
Version: 6.2.0
...

$ pip-sync --version
Traceback (most recent call last):
  File ".../.pyenv/versions/hq/bin/pip-sync", line 5, in <module>
    from piptools.scripts.sync import cli
  File ".../.pyenv/versions/3.9.7/envs/hq/lib/python3.9/site-packages/piptools/scripts/sync.py", line 12, in <module>
    from pip._internal.utils.misc import get_installed_distributions
ImportError: cannot import name 'get_installed_distributions' from 'pip._internal.utils.misc' (.../.pyenv/versions/3.9.7/envs/hq/lib/python3.9/site-packages/pip/_internal/utils/misc.py)

was having issues locally with previous version
@dannyroberts
Copy link
Member Author

dannyroberts commented Jan 11, 2022

Why didn't the one in test-requirements.txt get updated?

Sorry that was an oversight. I couldn't figure out a better way to do this than updating it manually in each place, and I just made a manual error doing that. Amended the commit

@dannyroberts
Copy link
Member Author

Yeah you're right, this doesn't work with pip-tools==6.2.0. I'll upgrade that as well.

to be compatible with the latest version of pip
@millerdev
Copy link
Contributor

millerdev commented Jan 11, 2022

Looks like pip-tools changed its output format, so probably necessary to run make requirements.

I'm also surprised by changes similar to this one in requirements.txt files:

@@ -250,9 +248,7 @@ jwcrypto==0.8
  kafka-python==1.4.7
      # via -r base-requirements.in
  kombu==4.2.2.post1
-    # via
-    #   -r base-requirements.in
-    #   celery
+    # via -r base-requirements.in
  laboratory==0.2.0
      # via -r base-requirements.in
  lxml==4.6.3
@@ -358,7 +354,6 @@ python3-saml==1.12.0
  pytz==2020.1
      # via
      #   -r base-requirements.in
-    #   celery
      #   django
      #   twilio

That looks like pip-tools dependency resolution may have changed for dependencies specified as tarball URLs (this possibly indicates a bug in the new version of pip-tools).

@dannyroberts
Copy link
Member Author

Huh, I guess this is a bigger change than I thought it would be, so I'm kind of stuck. I'll try and come back to this next week. If this gets in the way of anyone else before I do and/or wants to volunteer, don't hesitate!

@gherceg
Copy link
Contributor

gherceg commented Jan 18, 2022

In pip-tools 6.3.0, a change was made to generate direct references whenever possible was added. The explanation for favoring direct references can be found in the PR that initially added support:

In my projects which have git repositories specified as dependencies, without a direct reference or an egg, pip was not able to determine the package name, which was causing an issue during pip-sync (inside the merge, comparing two identical requirements from two different files).

Also, inside setup.py, install_requires must be written with a direct reference (pip fails to install the dependencies if it is not using a direct reference). pip-tools was ignoring the direct reference, so it was not generating the requirements.txt correctly.

Moving forward it appears this is the syntax we want our git repo dependencies to be formatted in.

@gherceg gherceg added the Open for review: do not merge A work in progress label Jan 18, 2022
@gherceg
Copy link
Contributor

gherceg commented Jan 19, 2022

That looks like pip-tools dependency resolution may have changed for dependencies specified as tarball URLs (this possibly indicates a bug in the new version of pip-tools).

Looks like the relevant bug here as been reported to pip-tools jazzband/pip-tools#1505 and/or jazzband/pip-tools#1518 and PRs with fixes have been open since the end of October. The most recent activity was 10 days ago with the original author asking for an update from reviewers. Optimistically, this should be resolved in the coming weeks.

The good news is I tested this in an isolated project with only celery and pip-tools as specified requirements, and sub-dependencies of celery like kombu were still added with the correct comment citing celery as the parent dependency. It appears this is only an issue when multiple dependencies include the same sub-dependency.

So while we wait for this issue to be fixed, do we want to manually update requirements.txt files to include any comments on sub-dependencies pip-compile may try to remove?

@millerdev
Copy link
Contributor

while we wait for this issue to be fixed, do we want to manually update requirements.txt files to include any comments on sub-dependencies pip-compile may try to remove?

I think manually updating .txt files will be too cumbersome and easy to forget. What do you think of temporarily documenting the omitted dependency comments in the relevant .in files? We can remove the comments when the issue is fixed.

I think there is little harm in omitting those comments from the .txt files while we wait for the issue to be fixed. It's discouraging that those issues and the fixes have been around since October and are still not fixed/merged. Hopefully that effort doesn't lose momentum.

Comment on lines 70 to 71
# missing celery specified as parent dependency in .txt files. Check https://github.com/jazzband/pip-tools/issues/1505
kombu
Copy link
Contributor

Choose a reason for hiding this comment

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

While I know this goes with kombu, it wasn't immediately obvious because I'm used to pip-tools convention of placing dependent comments indented below the dependency. What do you think of this?

Suggested change
# missing celery specified as parent dependency in .txt files. Check https://github.com/jazzband/pip-tools/issues/1505
kombu
kombu
# via celery
# See https://github.com/jazzband/pip-tools/issues/1505
# This comment can be removed when that issue is resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good suggestion. See 4f4d4a5

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Guess you decided not to indent the comments? (not a big deal)

Copy link
Contributor

Choose a reason for hiding this comment

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

I gotchu

@gherceg gherceg removed the Open for review: do not merge A work in progress label Jan 20, 2022
@gherceg gherceg merged commit b31d96a into master Jan 21, 2022
@gherceg gherceg deleted the dmr/upgrade-pip branch January 21, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants