Dealing with legacy version parsing functionality being removed

[hodgestar]

PR #407 dropped LegacyVersion, but at the same time it broke the API of packaging.version.parse in a way that makes it very hard to support parse from both packaging 21 and 22.

In 21 if one called parse("...") it would return LegacyVersion instead of raising an error. So if code wanted to use parse but check that the version was correct it has to check whether the returned type was LegacyVersion.

In 22, LegacyVersion is now gone, and an exception is raised instead.

The deprecations raised for LegacyVersion were only raised when a LegacyVersion was created, so code that called parse and checked the return type didn't raise any warnings.

[uranusjr]

so code that called parse and checked the return type didn't raise any warnings.

Could you clarify? The warning is emitted when a LegacyVersion is created, so if code checks the return value’s type, a warning would have been emitted when the return value is created in the first place. The reasoning does not make sense to me.

[hodgestar]

Maybe a code snippet will help make the issue clearer.

To use parse in 21 and 22, I ended up writing code like:

    version = packaging.version.parse(version_string)
    # LegacyVersion was removed in packaging 22, but is still returned by
    # packing <= 21
    LegacyVersion = getattr(packaging.version, "LegacyVersion", type(None))
    if isinstance(version, LegacyVersion):
        raise ValueError("invalid version: " + version_string)

In 22, the first line will raise an error (woot) but in 21 it won't.

[ldionne]

This actually breaks code that seems to make sense:

>>> import packaging.version
>>> packaging.version.parse('foo-123.0')
<LegacyVersion('foo-123.0')>

This doesn't work anymore.

It's actually worse than that. I thought this only broke because of the trailing .0, but this breaks for any version of that kind. Is there really a reason to even deprecate this useful functionality?

[pradyunsg]

The deprecations raised for LegacyVersion were only raised when a LegacyVersion was created, so code that called parse and checked the return type didn't raise any warnings.

parse creates LegacyVersion objects, and thus raises the deprecation warnings.

Try running with python -X dev or PYTHONDEVMODE=1.

There was a deprecation period for sure: #321

[hodgestar]

Could we please re-open this? I will try explain the problem in tiny minute detail.

Before, in packaging 21, one had to write code like this to parse a version and check that it was a valid version:

version = packaging.version.parse(version_string)
if isinstance(version, packaging.version.LegacyVersion):
    raise ValueError("invalid version: " + version_string)

because parse did not raise an error for a bad version.

The deprecation warning was not triggered, unless the exception case happened, which usually meant that someone had messed up the version number. Whether the deprecation was trigger or not is also somewhat moot since there is no other way to check that the result of parse was a valid non-legacy version.

Now in packaging 22, the equivalent code is just:

version = packaging.version.parse(version_string)

but that will silently accept bad versions in older versions of packaging, so one is left with awful code I resorted to my previous comment.

[hodgestar]

Try running with python -X dev or PYTHONDEVMODE=1.

There was a deprecation period for sure: #321

Replies like this are super unhelpful. I referenced the PR for #321in my bug report. I explained why the deprecation failed to be run in this sentence:

The deprecations raised for LegacyVersion were only raised when a LegacyVersion was created, so code that called parse and checked the return type didn't raise any warnings.

If this is the reaction of the packaging devs, I won't bother filing bug reports here in the future.

[pradyunsg]

Before, in packaging 21, one had to write code like this to parse a version and check that it was a valid version:

version = packaging.version.parse(version_string)
if isinstance(version, packaging.version.LegacyVersion):
    raise ValueError("invalid version: " + version_string)

No, you can do:

version = packaging.version.Version(version_string)

[pradyunsg]

The deprecations raised for LegacyVersion were only raised when a LegacyVersion was created, so code that called parse and checked the return type didn't raise any warnings.

If this is the reaction of the packaging devs, I won't bother filing bug reports here in the future.

Sorry, I guess I was a bit too terse there (I was on a train, and I was put off by a statement you made that was untrue; especially after a deprecation period that ran for over an year). Here's a fuller example:

❯ python -X dev
Python 3.11.0 (main, Oct 29 2022, 12:23:48) [Clang 14.0.0 (clang-1400.0.29.102)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import packaging
>>> packaging.__version__
'21.3'
>>> from packaging.version import Version, parse
>>> parse("legacyversion")
/Users/pradyunsg/Developer/github/packaging/.venv/lib/python3.11/site-packages/packaging/version.py:111: DeprecationWarning: Creating a LegacyVersion has been deprecated and will be removed in the next major release
  warnings.warn(
<LegacyVersion('legacyversion')>
>>> parse("1.0")
<Version('1.0')>
>>> Version("legacyversion")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/pradyunsg/Developer/github/packaging/.venv/lib/python3.11/site-packages/packaging/version.py", line 266, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: 'legacyversion'

Notice the DeprecationWarning being presented.

[brettcannon]

because parse did not raise an error for a bad version.

You could have gotten an error if you had turned on raising an exception for warnings when calling packaging.version.parse(), as Pradyun pointed out. But we were not going to deprecate that entire function when it works for people who provide it properly formatted versions as that would break way too much code that was already doing valid things with valid version numbers.

Replies like this are super unhelpful.

How so? You specifically said there wasn't a deprecation period and we are simply disagreeing with your sentiment by saying we provided a mechanism that either you didn't happen to know about or chose not to use. Are you specifically upset that Pradyun linked to the issue? You have to understand there are other people reading this issue, so even if you linked to the PR a direct link to the issue is still beneficial to others.

The deprecations raised for LegacyVersion were only raised when a LegacyVersion was created, so code that called parse and checked the return type didn't raise any warnings.

As Pradyun and I are arguing, though, that wasn't required of you to operate that way. Python's deprecation mechanism is the warnings module, and by default it prints to stderr. But you can (and I would argue should) run with warnings raised as exceptions in your tests to detect when you run into this situation as this is how deprecations are typically handled in Python code and by this project. And you can say that you happened to never trigger that warning until today and that's why you were caught off guard, but once again we were not comfortable ditching that entire function, so we did the best we could.

[pradyunsg]

I've retitled this to be something that won't annoy me when I revisit this, and reopened it, since that was requested by OP.

I sincerely don't think there's anything more we could've done here given that this is a low-level library that can't be spewing out to stdout/stderr and we need to rely on users of this library (which isn't the same as end users) doing their due deligence for dealing with deprecations. If you think there is, please say so without accusing us of not doing anything after we spent a not-insignificant amount of time figuring out how to make this as non-disruptive as possible (go read #321 as well as the PRs it led to, if you wanna see that).

[hroncok]

Perhaps a way to write the code that supports both versions of packaging is something like this:

with warnings.catch_warnings():
    warnings.filterwarnings("error", "Creating a LegacyVersion", DeprecationWarning)
    try:
        packaging.version.parse("legacyversion")
    except (DeprecationWarning, packaging.version.InvalidVersion):
        ...

[hroncok]

    LegacyVersion = getattr(packaging.version, "LegacyVersion", type(None))
    if isinstance(version, LegacyVersion):

This could probably be replaced with hasattr("major") which is true only for non-legacy versions.

[hroncok]

Perhaps what @hodgestar would appreciate is the ability to still run isinstance(version, packaging.version.LegacyVersion) even when LegacyVersions are no more. That could be achievable (if desired) by creating an empty LegacyVersion class that raises exception when inited but exists:

class LegacyVersion(_BaseVersion):
    def __init__(self, version):
        raise NotImplementedError(...)

[pradyunsg]

with warnings.catch_warnings():
    warnings.filterwarnings("error", "Creating a LegacyVersion", DeprecationWarning)
    try:
        packaging.version.parse("legacyversion")
    except (DeprecationWarning, packaging.version.InvalidVersion):
        ...

IIUC, You don't need to do all this. Most of this is functionally the same as doing packaging.version.Version("legacyversion") directly.

[hroncok]

IIUC, You don't need to do all this. Most of this is functionally the same as doing packaging.version.Version("legacyversion") directly.

Not if your code base needs to support both versions of packaging and raise an exception when a legacy version is provided.

[pradyunsg]

No, it will raise if given a legacy version, regardless of which version of packaging is being used. Version class has only ever accepted PEP 440 versions.

[hroncok]

Ah, I've missed that. I believe that is the most important bit of information that was perhaps lost in all the other: If you don't want to parse legacy versions, use packaging.version.Version instead of packaging.version.parse. Thanks!

[hodgestar]

version = packaging.version.Version(version_string)

This is exactly my point -- one can not use .parse because it's behaviour for the case of legacy version strings changed from 21 to 22.

[hodgestar]

Aside: Dealing with the pypa projects is super frustrating. This change broke every past release of my package and my setup.py file never raised any deprecation warnings because it only used LegacyVersion to check the legacy version case, which was only there because that was how parse worked in packaging 21. But instead of a "sorry, that could perhaps have been handled better", all I get is "this is all your fault, you should have known to use Version directly".

I get that people are trying to improve the ecosystem, but breaking packages every few months is painful, and it would be good for the people working in the packaging space to know that pain, even if they decide in the end that it is a pain that has to be born to build a better future.

[pradyunsg]

Feedback taken. Did you read #631 (comment)?

[hodgestar]

Feedback taken. Did you read #631 (comment)?

Of course.

[hodgestar]

@pradyunsg @brettcannon This bug report has done it's job. I've expressed what I wanted to. I don't feel that you understood why the change in behaviour of parse caused a minor mess, but I've also explained as best I can now.

Thank you for replying and doing your best too. I'll close this issue now and we can all move on with our lives.

[pradyunsg]

"sorry, that could perhaps have been handled better"

Then you know that I've nearly said this and that I don't know of a better way to handle this. If you think there is, please feel welcome to let us know.

it would be good for the people working in the packaging space to know that pain

The underlying assumption here is that we're not aware of the pain this can cause to users -- and, honestly, we are. This change has been 8+ years in the making (with ~2 years of that presenting a user-facing warning) where at each step we've been going "ok, what can we do to reduce disruption". We've made changes in the higher-level tools to help flag this to users more explicitly and this low-level library can't do much more than use language features for deprecating things that are due for removal.

[hodgestar]

As a concrete suggestion, I think it would have been better to deprecate .parse too, so that just calling .parse raised a deprecation warning.

[pradyunsg]

/me nods. That's unlikely to happen.

Since I'm seeing users cross-reference this issue, I'll cross reference the other relevant issues around this: #321 and #530.

[pradyunsg]

>>> packaging.version.parse('foo-123.0')
<LegacyVersion('foo-123.0')>

This doesn't work anymore.

It's actually worse than that. I thought this only broke because of the trailing .0, but this breaks for any version of that kind. Is there really a reason to even deprecate this useful functionality?

Oh, I never responded to @ldionne's comment: This broke because "foo-<version>" isn't a valid version -- you should pass in <version> directly.

This sort of issues have been a major source of confusion in and around Python packaging, so strings like that are literally why it was important to remove this functionality, and move to a world where such "not a version" strings are not accepted.

❯ echo "this is a random string@#%foo" | python -c "from packaging.version import parse; print(repr(parse(input())))"
<LegacyVersion('this is a random string@#%foo')>

vs

❯ echo "this is a random string@#%foo" | python -c "from packaging.version import parse; print(repr(parse(input())))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/pradyunsg/Developer/github/packaging/.venv/lib/python3.11/site-packages/packaging/version.py", line 52, in parse
    return Version(version)
           ^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/packaging/.venv/lib/python3.11/site-packages/packaging/version.py", line 197, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: 'this is a random string@#%foo'