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

Distutils Version classes (including LooseVersion) are deprecated #887

Closed
timokau opened this issue Aug 23, 2022 · 8 comments
Closed

Distutils Version classes (including LooseVersion) are deprecated #887

timokau opened this issue Aug 23, 2022 · 8 comments

Comments

@timokau
Copy link
Contributor

timokau commented Aug 23, 2022

The LooseVersion class (see #579) is now deprecated (distutils commit, setuptools changelog), which results in warnings of the form:

DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    if LooseVersion(sklearn.__version__) >= '0.22.0':

We should probably follow the suggestion and move to packaging.version.

@BenjaminBossan
Copy link
Collaborator

I didn't know that, thanks for bringing it up.

Is packaging part of the standard Python distribution or an extra dependency for all our supported Python versions? It seems not to exist for < 3.8, could it be? If so, I guess we would need to write the code in a way to still use LooseVersion for old Python versions.

@timokau
Copy link
Contributor Author

timokau commented Aug 23, 2022

I'm not sure, but as far as I know it's an extra dependency but it should be available for python >= 3.6 according to pypi.

@thomasjpfan
Copy link
Member

thomasjpfan commented Aug 23, 2022

Using packaging.version is known to be slow: pypa/setuptools#926

For Python >= 3.8, the best way to get versions is: importlib.metadata. Since we still support 3.7, there is a shim on pypi.

If we do not want to add another dependency, we can vendor the code for packaging.version. For example, this is how SciPy vendors it: https://github.com/scipy/scipy/blob/main/scipy/_lib/_pep440.py

@BenjaminBossan
Copy link
Collaborator

Why is something that is seemingly simple so complex? :(

For Python >= 3.8, the best way to get versions is: importlib.metadata.

One difference would be that this would compare strings, whereas with LooseVersion, we get a bit of a more sophisticated logic. E.g.:

LooseVersion(sklearn.__version__).version # => [1, 2, 'dev', 0]

Maybe it makes no difference for what we use, but I would much prefer a solution that uses the existing logic to compare versions.

For example, this is how SciPy vendors it: https://github.com/scipy/scipy/blob/main/scip

The link is broken but in general, if we're just dealing with a few lines of code, I'd prefer vendoring to adding a new dependency.

@thomasjpfan
Copy link
Member

My link got cut out. Here is the full link: https://github.com/scipy/scipy/blob/main/scipy/_lib/_pep440.py

@BenjaminBossan
Copy link
Collaborator

The scipy code looks good and should be battle tested. I'm not to happy that there seems to be no better option, but vendoring that code gets a +1 from me.

BenjaminBossan added a commit that referenced this issue Aug 26, 2022
Remove version checks that are no longer required because we don't
support the min versions anymore.

I was working on the proposed solution in #887 when I noticed that none
of the version checks we do is actually required anymore. After removing
them, we thus no longer depend on distutils's LooseVersion.

If we ever need to do a version check in the future, I would still go
with the scipy Version class.

Also, in the future, we should ensure to add #TODO comments to all
version checks so that we can easily find them.

One change I introduced here is to require scikit-learn>=1.0.0. I think
this should be fine because it's been out for some time now and it still
supports Python 3.7.

We could go to 0.24 instead and would still not need a version check,
let me know if you prefer that.
@BenjaminBossan
Copy link
Collaborator

With the module copied from scipy, we should be good, so let's close for now.

@timokau
Copy link
Contributor Author

timokau commented Sep 21, 2022

Thank you!

BenjaminBossan added a commit to BenjaminBossan/skops that referenced this issue Sep 26, 2022
For that Hub version, the use of the "token" argument produces a
FutureWarning, leading our CI to fail. This PR fixes the warnings while
leaving everything the same on the user side (i.e. they don't need to
change the "token" argument).

Implementation

As discussed, we will not change the argument name on the skops,
preferring token to use_auth_token.

For the version checking, I went with the private Version class from
scipy, which follows PEP440. This follows the approach taken in
skorch (see this issue:
skorch-dev/skorch#887), though we import it
here instead of vendoring. Let me know if another approach is preferred.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants