Surface LegacyVersion and LegacySpecifier deprecation warnings

[sbidoul]

This is a simple approach to surface LegacyVersion and LegacySpecifier deprecation warnings, by patching our vendored copy of packaging. I open it to support discussion only. If it ends up to be valid, it would need to be converted into a vendoring patch.

towards #12063
refs #11715

There are two concerns with this approach

1. It is not compatible with our vendoring policy (as mentioned by @pradyunsg in Upgrade the vendored packaging to 22.0+ #11715 (comment)).
2. The warnings may lack context.

Regarding 1, it may be debatable, since we are "just" improving an existing deprecation warning and making it more visible?

Regarding 2, I'd need more test cases to see if there is enough context provided by surrounding messages or not. Please share them here if you have some. A simple example 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 compatible version specifiers.
Requirement already satisfied: pip>23.0.* in /home/sbi-local/.virtualenvs/tempenv-16fa522829ba/lib/python3.8/site-packages (23.1.dev0)

[sbidoul]

Related:

pip/src/pip/__main__.py

Lines 23 to 28 in 5d4a974

	# Work around the error reported in #9540, pending a proper fix.
	# Note: It is essential the warning filter is set *before* importing
	# pip, as the deprecation happens at import time, not runtime.
	warnings.filterwarnings(
	"ignore", category=DeprecationWarning, module=".*packaging\\.version"
	)

[sbidoul]

I tend to think we should do this in 23.1 rather than postponing.

Users of debundled distributions that don't have the packaging patch will see the warning only when raising PYTHONWARNINGS level. But if they don't, no harm is done.

Regarding the context, we can direct users to the deprecation issue that recommends raising verbosity, which hopefully should give enough context.

[pfmoore]

The lack of discussion here, combined with the fact that 23.1 is due tomorrow, makes me feel like this isn't ready for 23.1. Sorry, I'd really like to get this resolved, but my understanding is that this will have enough of an impact that we can't simply drop it in and see what happens.

Personally, I'd like to just do this and accept the consequences. Yes, some users will be impacted, but we should take the position that they need to fix their projects. It's not up to pip to validate version numbers - projects using legacy versions should have been planning to transition for some time. But human nature being what it is, they won't have, and so pip is providing a 3-month "you need to fix this or things will break" warning to make sure they are aware, as per our deprecation policy. I think that's a reasonable thing to do and we should stick by our policy here.

But, based on past experience, it will cause complaints. And as such, I think we need to agree on this approach as a project. Specifically, if the warning causes problems for people, I expect with my RM hat on to say "sorry, this is a normal deprecation, deal with it". I explicitly don't want to revert the warning just because it's causing issues, and if we're not comfortable with that, then I think we need to address that disagreement before releasing the warning.

If we're not sure how much impact this will have, and that matters to us, then I think we need to work out how to collect that information properly, not by tentatively making the change and seeing if people complain too loudly. But that likely means looking at the question of telemetry, and opens up a whole other bunch of difficult issues, that I don't want to have the packaging upgrade blocked by. So I think we should continue with our current approach of making the best decision we can based on the limited information available, at least for this issue.

There's also a couple of technical issues to address:

1. The PR is failing CI (and is still in draft state). It's definitely not going in unless that gets fixed.
2. Do we know if this will pass the CPython test suite? We know they run with warnings as errors. I don't want to release 23.1 and then find we can't upgrade the CPython bundled version because of this.

[sbidoul]

Hi Paul, I'm not sure I get your point.

I think we all agree we need a deprecation cycle for this.

What I was trying to say is that I believe we'd better land an imperfect warning in 23.1 than waiting one more release to start the deprecation. The worse that can happen IMO is that we'll get questions of confused users because the warnings lack context and they don't know where the deprecated versions come from.

So the only thing we need to discuss here is whether the way this PR implements the warning is acceptable or not.

If it is, then we just need to convert this PR into a vendoring patch which should be easy to do.

The PR is failing CI

Because it is not a proper vendoring patch and it lacks a changelog entry. The rest is green.

(and is still in draft state).

As I said, I opened this to support the discussion.

Do we know if this will pass the CPython test suite? We know they run with warnings as errors. I don't want to release 23.1 and then find we can't upgrade the CPython bundled version because of this.

I don't know. We don't usually test pip within the cpython test suite before release, as far as I know. This PR, however, converts a python warning into a pip deprecation warning, so I guess it should be fine?

My 2 cents. Just trying to help. I don't personally have an urgency to upgrade our vendored packaging.

[pfmoore]

OK, sorry I might have misunderstood the status of this PR, based on the fact that you added it to the 23.1 milestone.

What are you expecting needs to happen to get this into 23.1? I assume you want someone to say that patching the vendored code like this is OK? It's fine by me, but I'm not clear what @pradyunsg's concerns are.

I agree I may be worrying too much about the CPython tests. Let's assume the best, we can fix things if there's a problem.

As long as we're all clear that once we start the deprecation, we're going through with it (I don't want another situation like dependency_links) then I guess it's just a question of managing user reactions (and again, I'm not that familiar with the risks here, I'm mostly just assuming that because we've hesitated so long on this, it's because there's an expectation somewhere that it might have a relatively large impact...)

Also, what (if anything) do you want from me as RM? Do you need a delay in 23.1?

[pradyunsg]

I'd say we can defer this by 3 months; instead of trying to rush it into the 23.1 release. There's little to no reason for us to take on additional stress to hit a deadline here.

I'm not clear what @pradyunsg's concerns are.

We know that our code is debundled by downstream users and I'd prefer to not have deprecation patches be something we inject into our dependencies. What we tend to have are patches for making stuff run or vendorable, rather than new functionality or behaviour changes.

Behaviour changes mean we're effectively maintaining a fork of the dependency and that's not a particularly sustainable ways to doing things for us.

[sbidoul]

Ok, perfect. I've removed this from the 23.1 milestone.

As long as we're all clear that once we start the deprecation, we're going through with it

In my understanding, we have no choice. Since this blocks the packaging upgrade, we'll have to do this sooner or later.

[pfmoore]

Ok, perfect. I've removed this from the 23.1 milestone.

Thanks. Sorry for pushing back this hard, but I'd rather we do it right than too soon.

Since this blocks the packaging upgrade, we'll have to do this sooner or later.

Absolutely. I'd just like to avoid associating "enforcing standards" with user pain and frustration as much as we can.

[sbidoul]

Looking back at this, I still think this PR, when modified to be a proper vendoring patch, is a reasonable and simple way to surface warnings about LegacyVersion and LegacySpecifier. While I do understand the theory of not modifying vendored dependencies I don't quite see how it is a problem in this case, since all it does is tweaking warning.

[pfmoore]

Agreed, I'm fine with this going in (and I think that early in the 23.2 cycle is the right time). And I don't think we should worry about it not technically being in line with our patching policy - it's a temporary change to ensure we warn of a deprecation, and that seems perfectly reasonable to me.

[sbidoul]

Alright, so:

• I made this a proper vendoring patch
• I created the deprecation issue Deprecate legacy versions and version specifiers #12063 (feel free to improve the text over there)
• I set gone_in 23.3 (we can still postpone if we get a lot of feedback? or should it be 24.0 from the start)

If anyone has the appetite to propose a more elaborate deprecation mechanism, we can still do so, but at least when this is merged we'll start receiving feedback.

[sbidoul]

Does anyone know a public example with non-standard version numbers in dependencies? Before merging I'd like to test a few more cases than the trivial one shown in the OP, in particular to check if there would be a flood of identical warnings.

[uranusjr]

No immediate ones, but maybe look for some dependants to pytz? That’s a popular pacakge with non-standard versions until relatively recently. https://www.wheelodex.org/projects/pytz/rdepends/

[sbidoul]

Hm, so a simple pip install pytz logs a warning for each legacy version of pytz on PyPI then installs the last one. So these warnings are not going to be very useful for the user. I'm setting this back to draft.

[sbidoul]

Not sure how to handle this. Perhaps should we warn only when the resolver ends up selecting a legacy version?

[pfmoore]

To confirm, what will be the final result here? Once the deprecation becomes an error, will we simply ignore the legacy versions? If so, then maybe checking how many versions are invalid for each package and adding a warning saying something like "20 non-standard versions of pytz found, these will be ignored in future"?

It will make the reporting a lot more complex, and it's still not anything that needs action from the user unless an invalid version gets installed, but maybe it's useful information anyway, just to indicate how many invalid versions are out there?

The other thing we could do is have pip check (which I believe is run after any completed install?) validate whether all installed packages have valid version numbers. A message like "Your system now has XX non-standard versions installed, pip will no longer support these after version XX.Y". That would be something directly actionable by the user.

[sbidoul]

Once the deprecation becomes an error, will we simply ignore the legacy versions

That's what I'm assuming.

What we could do is warn if any LegacyVersion is selected by the resolver (in pip install, pip wheel and pip download). I pushed a rough commit to show what it could look like in pip install.

To test that I'd very much welcome examples of packages with legacy versions on PyPI that are actually installable with a recent pip.

What about the legacy specifiers? When they are part of top level requirements, we will error out, I guess? But what will the resolver do legacy specifiers are found as part of Requires-Dist metadata?

[sbidoul]

I also pushed a commit with the deprecation in pip check.

[sbidoul]

Interestingly recent versions of wheel and setuptools both vendor a recent packaging, so they can't build wheels with non standard versions and specifiers.

And PyPI has been validating PEP 440 for ages (dixit @dstufft), and also has a recent packaging.

So could it be we are worrying too much?

Anyway I updated this PR to warn

• after resolving
• in post-install conflict detection
• in pip check

This is ready to review again. Let's land this in 23.2.

[dstufft]

I looked it up, at least 8 years now, basically Warehouse has never allowed a version to be uploaded that wasn't PEP 440 compliant.

The old legacy codebase is too hard to code delve in to see if we validated PEP 440 there or not :)

[uranusjr]

Seems like there’s a test failure. Not sure what’s going on.

[XPASS(strict)] Unexplained 'no module named platform' in https://github.com/pypa/wheel/blob/c87e6ed82b58b41b258a3e8c852af8bc1817bb00/src/wheel/vendored/packaging/tags.py#L396-L411

[sbidoul]

Seems like there’s a test failure. Not sure what’s going on.

[XPASS(strict)] Unexplained 'no module named platform' in https://github.com/pypa/wheel/blob/c87e6ed82b58b41b258a3e8c852af8bc1817bb00/src/wheel/vendored/packaging/tags.py#L396-L411

I handled this in #12109. Not that I understand the cause any better than before, though :)

[pfmoore]

@sbidoul is this good to merge? You have 2 approvals, and the tests are passing.

[sbidoul]

It should be ready yes. The approvals were for a completely different approach, though. I'm away from the keyboard for two weeks so feel free to merge but note I will not be able to help with any issue until mid July.

[pfmoore]

Ah, I hadn't seen it was a different approach. I've just checked the new code and I'm OK with it. But I don't have the free time to commit to handling issues either, so I'm not going to merge it myself. @pradyunsg @uranusjr are either of you OK with merging this? Worst case, if any issues arise we can revert and push it back to 23.3, but I'd rather we unblock the packaging upgrade, so my preference is to have someone looking after it post-merge.

[pradyunsg]

I should have the bandwidth to pick up any issues. Thanks @sbidoul for driving this! ❤️