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 the vendored packaging to 22.0+ #11715

Closed
1 task done
pradyunsg opened this issue Jan 9, 2023 · 14 comments · Fixed by #12300
Closed
1 task done

Upgrade the vendored packaging to 22.0+ #11715

pradyunsg opened this issue Jan 9, 2023 · 14 comments · Fixed by #12300
Labels
type: deprecation Related to deprecation / removal. type: maintenance Related to Development and Maintenance Processes
Milestone

Comments

@pradyunsg
Copy link
Member

pradyunsg commented Jan 9, 2023

Filing an issue to track this since there's a lot of pieces involved as far as I can tell.

xref pypa/packaging#530

Needs:

Blocks:

@pradyunsg pradyunsg added the type: maintenance Related to Development and Maintenance Processes label Jan 9, 2023
@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 9, 2023

A concensus on the deprecation plan for legacy versions / requirements + rollout

For this, my thought process is that we do this in the span of a 3 month deprecation before removal; where pip would warn on every Requirement / Version that we create -- a deprecation warning about the LegacyVersion/LegacySpecifier being used and the source string that triggers it.

One behaviour that I'm flip-flopping on: Should we skip wheels / sdists that contain requirements with this value in the dependency resolver, or should we reject them with an error?

@pradyunsg pradyunsg changed the title Upgrade the vendoring packaging to 22.0+ Upgrade the vendored packaging to 22.0+ Jan 14, 2023
@nanonyme
Copy link

My two cents is if they are the best version that can be resolved, error makes sense. If best version is something else, ignore. Does that make the best version resolution too slow?

@timfel
Copy link

timfel commented Mar 17, 2023

Just a note, since https://github.com/pypa/packaging/pull/607/files was merged and wheel and meson-python have updated to include those changes, locally built packages can no longer be installed on GraalPy unless we downgrade wheel and meson-python. The linked PR changed the abi tag for GraalPy. Just leaving this note here to say I wouldn't mind if pip updated it's vendored packaging at some point :)

@sbidoul
Copy link
Member

sbidoul commented Apr 9, 2023

I was (briefly) looking at deprecating LegacyVersion and LegacySpecifier.

Doing it in pip sounds non trivial. One thing that may be easier is to surface the two deprecation warnings that are already present in our vendored copy of packaging. The downside of that is that the warning messages would possibly lack context.

One hurdle I face, though, is actually finding examples of usages of these that actually work with pip main. If anyone can provide some that would be helpful.

@pradyunsg
Copy link
Member Author

pradyunsg commented Apr 9, 2023

Oh! We bumped pkg_resources in d7e0248, which might be why certain failures aren't happening?

python -X dev -m pip install "pip>=23.0.*"
/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/pkg_resources/__init__.py:121: DeprecationWarning: pkg_resources is deprecated as an API
  warnings.warn("pkg_resources is deprecated as an API", DeprecationWarning)
/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/packaging/specifiers.py:255: DeprecationWarning: Creating a LegacyVersion has been deprecated and will be removed in the next major release
  warnings.warn(
Requirement already satisfied: pip>=23.0.* in ./.venv/lib/python3.11/site-packages (23.1.dev0)

Running with python -X dev does expose those warnings FWIW. None of the warnings print the actual version string or its source though.

@sbidoul
Copy link
Member

sbidoul commented Apr 9, 2023

Running with python -X dev does expose those warnings FWIW. None of the warnings print the actual version string or its source though.

We can patch these messages in our vendored packaging. With your example it looks like this:

pip install --dry-run "pip>=23.0.*"
DEPRECATION: This form of version specifier (>=23.0.*) has been deprecated. pip 23.2 will enforce this behaviour change. A possible replacement is use PEP 440 compliant version specifiers.
DEPRECATION: This version format (23.1.dev0) has been deprecated. pip 23.2 will enforce this behaviour change. A possible replacement is use PEP 440 compliant versions.
DEPRECATION: This version format (23.0.*) has been deprecated. pip 23.2 will enforce this behaviour change. A possible replacement is use PEP 440 compliant versions.
Requirement already satisfied: pip>=23.0.* in /home/sbi-local/.virtualenvs/tempenv-11a6272805892/lib/python3.8/site-packages (23.1.dev0)

A bit chatty, but we could silence repetitions okayish?

Why does it complain about 23.1.dev0 though? It is my pip dev version that is installed. But isn't that a valid PEP 440 version?

@pradyunsg
Copy link
Member Author

pradyunsg commented Apr 9, 2023

I think the most important piece is to show comes_from information with the warnings FWIW, and to show the warning after the parse rather than during it. The problematic specifier/version is perhaps as important to present to the user as "it came from package X's runtime dependencies, and this is the specification" or "it came from package X's sdist build" or things like that.

#10550 function made some process toward reducing the complexity of implementing this on pip's end FWIW. parse_req_from_editable and load_pyproject_toml are the only two functions that we would need to migrate to that IIUC; and that removes the need to rework the specifier warnings with a patch.

Other than that, with 0fcd11f, we can create a similar function to wrap packaging.version.parse, call it parse_version and provide warnings for versions as well through that helper.

@pradyunsg
Copy link
Member Author

I'm wary of modifying our dependencies' source code to accomplish this FWIW -- the first bullet in our vendoring policy is...

Vendored libraries MUST not be modified except as required to successfully vendor them.

And, notably, it would mean that downstream debundlers would have a copy of pip that won't behave as the same copy; because it won't have our patch. I am open to monkeypatching packaging though or wrapping our internal calls to it, so that we can present this information without needing to change the behaviour of the underlying code! I'm also open to patching warnings.showwarning to show this warning. :)

@pfmoore
Copy link
Member

pfmoore commented Apr 10, 2023

Quick update - where are we on this change?

I'm assuming this (i.e., the change to update our vendored packaging) won't be going into 23.1. Is there any PR yet for the warnings that we need to get in place for the start of the deprecation cycle leading to the revendoring? I'm willing to be flexible with 23.1 to get that preparatory work in place, but if we don't even have a PR for it yet, is it realistic? Would a delay of a week (to 22/23 Apr) in 23.1 be of any practical help? (I have other personal reasons for being willing to slip a week, so it's not just for this work...)

Ideally, can we either decide we're going to miss 23.1, or get a PR/issue created that can be put on the 23.1 milestone?

@sbidoul
Copy link
Member

sbidoul commented Apr 10, 2023

@pfmoore I opened #11945 with my current prototype, to support the discussion.
I will unfortunately not have time to implement a more sophisticated solution in time for 23.1, even with one more week.

@pradyunsg
Copy link
Member Author

@uranusjr and I spent some time chatting about this during PyCon and he spent some cycles looking into this.

@pradyunsg
Copy link
Member Author

The concensus on this seems to be that we'll deprecate this and remove this (maybe in an expedited manner) and move ourselves forward with that. The current plan is to implement a wrapper function to handle caching and also the deprecation warning.

@pradyunsg
Copy link
Member Author

@uranusjr I've locked #12012 eagerly, to avoid noise on the issue until after the release is out. Feel welcome to unlock if that's not in line with what you have planned. :)

@mayeut
Copy link
Member

mayeut commented Dec 14, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: deprecation Related to deprecation / removal. type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants