ENH: Use importlib in minversion by pllim · Pull Request #11714 · astropy/astropy

pllim · 2021-05-06T22:11:08Z

Description

This pull request is to simplify minversion code to use importlib.metadata.version that Python provides.

github-actions · 2021-05-06T22:11:51Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2021-05-06T22:23:06Z

astropy/utils/tests/test_introspection.py

-    test_module.__version__ = '0.12.2'
-    good_versions = ['0.12', '0.12.1', '0.12.0.dev', '0.12dev']
-    bad_versions = ['1', '1.2rc1']
+    import numpy


I could not use a fake module anymore with this change, but I did test edge case manually. If there is a way to pass in fake module again, please let me know. Thanks!

With this patch:

>>> from astropy.utils import minversion >>> import numpy >>> numpy.__version__ '1.20.2' >>> minversion(numpy, '1.20.2') True >>> minversion(numpy, '1.20.2', inclusive=False) False

mhvk

Wow, I had looked at this and had thought maybe to just add an extra layer, but you just went what does seem the right route, and hacked it all out. I like it! Just an in-line comment about how to get importlib.metadata on python 3.7

mhvk · 2021-05-06T22:41:54Z

astropy/utils/introspection.py

 import sys
 import types
-import importlib
+import importlib.metadata


importlib.metadata requires python 3.8, so this needs to be

try: from importlib import metadata except ImportError: import importlib_metadata as metadata

(Unless we drop python 3.7, which would be borderline: it was released June 27, 2018, so we're supposed to support 42 months, i.e., until Dec 27, 2021...)

Ah, I knew I missed something. Thanks!

pllim · 2021-05-06T23:12:46Z

astropy/utils/tests/test_introspection.py

-    bad_versions = ['1', '1.2rc1']
+    import numpy
+    good_versions = ['1.16', '1.16.1', '1.16.0.dev', '1.16dev']
+    bad_versions = ['100000', '100000.2rc1']


p.s. Need to update this test when Numpy v100000 is released. 😆

mhvk

Looks good to me! Might still be good for @embray to have a look as well.
EDIT: dismissed my earlier approval since tests are still failing

mhvk · 2021-05-07T00:56:11Z

Well, it may look good but the tests failed quite miserably!?

mhvk · 2021-05-07T01:19:47Z

Hmm, the problem is that metadata.version expects a distribution name, but for yaml that is pyyaml, not yaml. It is quite unclear how to go backwards from that...

Tests are still failing, this would imply larger API change

saimn · 2021-05-07T06:49:33Z

Indeed, for some packages (pyerfa too) the import name is not the package name ...

mhvk · 2021-05-07T13:35:50Z

@saimn - any suggestion on how to get the package name from the module? Or perhaps we should stick with supporting __version__ as well?

saimn · 2021-05-07T14:39:02Z

It seems a function was added recently to do just this: packages_distributions()
https://importlib-metadata.readthedocs.io/en/stable/using.html#package-distributions
But I guess it will be in Python 3.10, so we would have to require the backport package for all Pythons < 3.10

Otherwise I found a way with dist.read_text('top_level.txt') and wasn't sure if it was a hack or not. packages_distributions() uses that (https://github.com/python/importlib_metadata/pull/287/files), so I guess that's a solution.

pllim · 2021-05-07T14:49:52Z

Does this mean switching to importlib here is currently pre-mature? We can always wait a few more releases.

encukou · 2021-08-05T11:24:40Z

astropy/utils/introspection.py

+    module_version = metadata.version(module_name)
    if inclusive:
-        return LooseVersion(have_version) >= LooseVersion(version)
+        return module_version >= version


importlib.metadata.version returns a string, not a version object like LooseVersion or packaging.version.Version. Strings don't compare well as versions, e.g. '3.10' < '3.9'.

pllim · 2021-08-05T13:09:32Z

This has languished too long. I don't remember what is the status anymore...

saimn · 2021-09-06T17:07:47Z

We would need a function similar to packages_distributions mentioned above, we could just copy it from https://github.com/python/importlib_metadata/pull/287/files (it's only 5 lines) until 3.10 is our minimum version.
And @encukou is right, version string should be parsed with packaging.version.Version before being compared.

pllim · 2021-09-07T14:36:20Z

I am not sure if I have time... @saimn or @encukou , do you want to submit a new PR to supersede this?

saimn · 2021-09-08T17:11:36Z

@pllim - I pushed a new commit to your branch with the changes mentioned above. One drawback with packages_distributions is that it is a bit slow, so we may want to cache it's output.

astropy/utils/introspection.py

mhvk

The need for the function and the cache are not quite as optimal as hoped (see suggestion inline for removing the cache), but I think it is definitely good to get rid of any use of distutils. And it seems that now we gotten as far as we did, we may as well go with it.

mhvk · 2021-09-08T19:34:21Z

astropy/utils/introspection.py

-    m = re.match(expr, have_version)
-    if m:
-        have_version = m.group(0)
+    # Convert the module name to a distribution name


I think we mostly do not need this dict, since for most packages the name and distribution name are the same. So, might it make sense to generally avoid creating it by just doing something like:

try: module_version = metadata.version(module_name) except metadata.PackageNotFoundError: # module name different from package name? module_version = metadata.version(packages_distributions()[module_name][0])

Good idea 👍

pllim

I cannot approve my own PR but the updates LGTM. Thank you very much, @saimn !

mhvk

Looks good to me too!

mhvk · 2021-09-08T20:20:54Z

Code coverage we can ignore, so fine to merge once the last test has passed.

pllim · 2021-09-08T20:32:59Z

Thanks, all!

pllim added Refactoring API change PRs and issues that change an existing API, possibly requiring a deprecation period labels May 6, 2021

pllim added this to the v5.0 milestone May 6, 2021

pllim requested review from embray, mhvk and saimn May 6, 2021 22:11

github-actions bot added the utils label May 6, 2021

pllim force-pushed the utils-minversion-importlib branch from 8841ce6 to 962f99d Compare May 6, 2021 22:14

pllim commented May 6, 2021

View reviewed changes

pllim marked this pull request as ready for review May 6, 2021 22:23

This comment has been minimized.

Sign in to view

mhvk reviewed May 6, 2021

View reviewed changes

pllim commented May 6, 2021

View reviewed changes

mhvk previously approved these changes May 7, 2021

View reviewed changes

mhvk self-requested a review May 7, 2021 01:37

saimn mentioned this pull request Jul 20, 2021

Fix warnings with Python 3.10 #11962

Merged

9 tasks

encukou reviewed Aug 5, 2021

View reviewed changes

pllim added 2 commits September 8, 2021 18:09

ENH: Use importlib in minversion

2103874

Fix importlib import

00bd292

pllim and others added 2 commits September 8, 2021 18:09

Add back missing import

dc20335

Use packages_distributions to find the distribution name

3f740e3

saimn force-pushed the utils-minversion-importlib branch from 0b716af to 3f740e3 Compare September 8, 2021 17:08

Cache packages_distributions' output

16b2f4c

pllim commented Sep 8, 2021

View reviewed changes

astropy/utils/introspection.py Outdated Show resolved Hide resolved

mhvk reviewed Sep 8, 2021

View reviewed changes

Use packages_distributions only if needed

1d8f93d

pllim commented Sep 8, 2021

View reviewed changes

pllim added the Ready-for-final-review label Sep 8, 2021

mhvk approved these changes Sep 8, 2021

View reviewed changes

pllim merged commit 9ebf553 into astropy:main Sep 8, 2021

pllim deleted the utils-minversion-importlib branch September 8, 2021 20:32

pllim mentioned this pull request Sep 9, 2021

TST: importlib usage broke pyinstaller cron job #12155

Closed

pllim mentioned this pull request Sep 27, 2021

add packaging to required packages #12199

Merged

9 tasks

pllim mentioned this pull request Nov 3, 2021

[5.0RC1] utils.minversion("astropy", "5.0") doesn't work (and seems unneeded?) #12377

Closed

pllim mentioned this pull request Oct 11, 2023

MNT: Removed deprecated version_path in minversion #15468

Merged

1 task

Uh oh!

Conversation

pllim commented May 6, 2021

Description

Uh oh!

github-actions bot commented May 6, 2021

Uh oh!

pllim May 6, 2021

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk May 6, 2021

Choose a reason for hiding this comment

Uh oh!

pllim May 6, 2021

Choose a reason for hiding this comment

Uh oh!

pllim May 6, 2021

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 7, 2021

Uh oh!

mhvk commented May 7, 2021

Uh oh!

saimn commented May 7, 2021

Uh oh!

mhvk commented May 7, 2021

Uh oh!

saimn commented May 7, 2021

Uh oh!

pllim commented May 7, 2021

Uh oh!

encukou Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

pllim commented Aug 5, 2021

Uh oh!

saimn commented Sep 6, 2021

Uh oh!

pllim commented Sep 7, 2021

Uh oh!

saimn commented Sep 8, 2021

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

saimn Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 8, 2021

Uh oh!

pllim commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mhvk left a comment •

edited

Loading