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

packaging import fix #7

Merged
merged 5 commits into from
Aug 28, 2024
Merged

packaging import fix #7

merged 5 commits into from
Aug 28, 2024

Conversation

stefan-anmut
Copy link

@stefan-anmut stefan-anmut commented Aug 23, 2024

When trying to run a git push on any of our pipenv-cookiecutter repositories, both of the pipenv-setup pre-push hooks fail with the following error:

Traceback (most recent call last):
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/bin/pipenv-setup", line 5, in <module>
    from pipenv_setup.main import cmd
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/lib/python3.12/site-packages/pipenv_setup/main.py", line 25, in <module>
    from pipenv_setup import (
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/lib/python3.12/site-packages/pipenv_setup/lockfile_parser.py", line 3, in <module>
    from requirementslib import Lockfile, Requirement
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/lib/python3.12/site-packages/requirementslib/__init__.py", line 4, in <module>
    from .models.lockfile import Lockfile
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/lib/python3.12/site-packages/requirementslib/models/lockfile.py", line 13, in <module>
    from .requirements import Requirement
  File "/Users/stefmd/.cache/pre-commit/repoa7rtv7ax/py_env-python3.12/lib/python3.12/site-packages/requirementslib/models/requirements.py", line 26, in <module>
    from pip._vendor.packaging.specifiers import (
ImportError: cannot import name 'LegacySpecifier' from 'pip._vendor.packaging.specifiers'

After some research, I discovered that this issue was due, at its very heart, to the fact that the packaging lib deprecated its LegacyVersion and LegacySpecifier functionality (read about that here) from version 22.0 onwards.

How this relates to this particular problem is twofold:

  1. packaging is used within the repo here, but THIS IS NOT THE PROBLEM since we pin its version in the pipfile to one that is compatible with version 21.0 which still contains the now deprecated functionality.
  2. packaging also just so happens to be sub-sub-dependency of pipenv-setup via the requirementslib package which depends on pip which then itself depends on packaging. THIS IS THE ROOT CAUSE OF THE PROBLEM.

Taking a dive down into this particular rabbit-hole, raises yet another twofold problem:

  • even the latest versions of requirementslib still depends on the LegacySpecifier functionality from packaging via pip, as can be seen in the GitHub source code here and the raised issue here.
  • and crucially, requirementslib will install ANY version of pip greater than or equal to 22.2 which basically renders the package useless given it will by default install the latest version which has been updated to cater for the update in packaging and therefore give the error above.

In other words, requirementslib need to sort their sh*t out.

So in order to fix:

  • I had to manually install pip and pin the version within the Pipfile to an older compatible version (v24.0) which still depends on the older version of packaging containing the LegacySpecifier functionality (v21.3), as recommended here, as this will force requirementslib to use that version of pip which isn't problematic.

Problem solved. For now.

Pipfile Show resolved Hide resolved
jmartin-anmut
jmartin-anmut previously approved these changes Aug 23, 2024
Copy link

@jmartin-anmut jmartin-anmut left a comment

Choose a reason for hiding this comment

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

I'm not a fan of adding pip to the Pipfile because of compatibility issues it might raise down the line, especially as pipenv uses its own version of pip... but in this scenario I don't see much choice. The only other option would be remove our dependency on requirementslib, but to be honest I don't know if there are other packages that would give us the same functionality, and might be more work than it's worth instead of just waiting for the issue with requirementslib to be fixed. It will only be a matter of time as other projects have done the same.

If we go ahead with this, we need to keep an eye on requirementslib so that when the issue is resolved, we can swiftly remove pip from the Pipfile. Maybe we create a separate GH issue for it so we don't forget.

Nice job figuring this one out!

@stefan-anmut
Copy link
Author

stefan-anmut commented Aug 23, 2024

I'm not a fan of adding pip to the Pipfile because of other compatibility issues it might raise down the line

That's a very good point James. Luckily we have all dependencies locked down to a certain version in the Pipfile so, as long as it works now (which it does), I'm pretty sure it will continue to work (I guess unless there are other "loose" sub-dependencies). Until the day we decide to start updating things, but we'll cross that bridge when we get to it.

And yep, also good point on raising an issue to address this in the future. I've created one as suggested (link).

Thanks for the review.

@csut-anmut
Copy link

The changes themselves look fine to me, although I'm cautiously apprehensive about pinning pip as this could cause us compatibility issues later (in other projects where pipenv-setup is installed as a hook or dependency, for example).

If I'm understanding correctly, the reason pip needs to be pinned is because the latest version of pip expects a fully updated version of packaging, which we can't use as requirementslib hasn't been updated to not use the now-deprecated LegacyVersion and LegacySpecifier functionality, right?

I echo James' sentiment that we should keep an eye on requirementslib and unpin pip if and when the issue is fixed.

@stefan-anmut
Copy link
Author

stefan-anmut commented Aug 28, 2024

Yes pretty much @csut-anmut.

But for the avoidance of any doubt, I'll try to explain it again:

The reason we need to pin pip is because requirementslib still uses old/deprecated packaging functionality via it's pip dependency (as can be seen from this import line in requirementslib here) BUT at the very same time also specifies to import any version of pip >= 23.1 (as can be seen here).

This is problematic because any version of pip > 24.0 will have removed this old/deprecated functionality by upgrading its own dependency on packaging.

So to fix, we have to force requirementslib to use a version of pip that still works (ie. 24.0), instead of the latest which it installs by default, by installing it and pinning its version in our Pipfile.

Hope that makes sense.

csut-anmut
csut-anmut previously approved these changes Aug 28, 2024
Copy link

@csut-anmut csut-anmut left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. Approved 👍.

Copy link

@jmartin-anmut jmartin-anmut left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@stefan-anmut stefan-anmut merged commit 878b495 into master Aug 28, 2024
@stefan-anmut stefan-anmut deleted the fix_packaging_import_issue branch August 28, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants