Skip to content

Use importlib.metadata instead of deprecated pkg_resources#2096

Merged
zklaus merged 8 commits into
mainfrom
use-importlib-metadata
Jul 6, 2023
Merged

Use importlib.metadata instead of deprecated pkg_resources#2096
zklaus merged 8 commits into
mainfrom
use-importlib-metadata

Conversation

@bouweandela

Copy link
Copy Markdown
Member

Description

Closes #2094


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@bouweandela bouweandela added the installation Installation problem label Jun 16, 2023
@codecov

codecov Bot commented Jun 16, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2096 (831f7db) into main (9d6b963) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##             main    #2096   +/-   ##
=======================================
  Coverage   93.09%   93.09%           
=======================================
  Files         237      237           
  Lines       12794    12795    +1     
=======================================
+ Hits        11910    11911    +1     
  Misses        884      884           
Impacted Files Coverage Δ
esmvalcore/_main.py 90.61% <87.50%> (-0.26%) ⬇️
esmvalcore/config/_config.py 100.00% <100.00%> (+1.08%) ⬆️

@bouweandela bouweandela marked this pull request as ready for review June 16, 2023 13:43
@bouweandela

Copy link
Copy Markdown
Member Author

I'm not able to reproduce the issue reported by Codacy locally and usage with a group keyword argument is exactly as in the examples on the docs page, so I'm not sure what is causing it.

@valeriupredoi valeriupredoi self-requested a review as a code owner June 22, 2023 12:36

@valeriupredoi valeriupredoi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bouweandela there is an issue with this implementation in Python 3.9, bud. I'll also have a look, possibly Codacy is complaining for the same reason

@valeriupredoi

Copy link
Copy Markdown
Contributor

first clue as for the fail issue - there are two slightly different packages installed (for all Python versions, not just 3.9):

 + importlib-metadata                    6.7.0  pyha770c72_0             conda-forge/noarch        26kB
 + importlib_metadata                    6.7.0  hd8ed1ab_0               conda-forge/noarch         9kB

this no bueno. Lemme dig deeper

@valeriupredoi

valeriupredoi commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

so importlib_metadata is an old package that was decomissioned by the maintainer in January, see https://anaconda.org/anaconda/importlib_metadata - and they migrated it to importlib-metadata which is at 6.7.0 now (whereas _metadata stopped at 6.0.0). We have two issues here:

  • we have a dep in our env that pulls importlib_metadata; we end up with both -metadata and _metadata; the import in module is obv with import importlib_metadata so that ends up importing the old package
  • conda forge is silly and reports that _metadata is at the same version 6.7.0 as -metadata (most prob a bug in the ABI) - this needs reported

@valeriupredoi

Copy link
Copy Markdown
Contributor

@zklaus what's the best way to report a broken package to conda-forge? Via gitter or open an actual issue? 🍺

@valeriupredoi

Copy link
Copy Markdown
Contributor

OK gotcha - iris needs both importlib_metadata and importlib-metadata (hopefully not via one of their deps)

@valeriupredoi

Copy link
Copy Markdown
Contributor

OK gotcha - iris needs both importlib_metadata and importlib-metadata (hopefully not via one of their deps)

it's in fact via dask and I stopped pruning environments and solving dep graphs there, I asked on Gitter first https://app.gitter.im/#/room/#conda-forge:matrix.org

@bouweandela

bouweandela commented Jun 22, 2023

Copy link
Copy Markdown
Member Author

Note that this module is part of the standard library: https://docs.python.org/3.11/library/importlib.metadata.html. I guess something changed between Python 3.9 and 3.10, though it isn't immediately obvious from the documentation. It looks like API docs are completely missing for the module.

@valeriupredoi

Copy link
Copy Markdown
Contributor

ah then I chased a red herring then 🐟 - still, for Python 3.9 and only for Python 3.9 there are two importlib-/_resources:

    + importlib-resources                  5.12.0  pyhd8ed1ab_0            conda-forge/noarch         9kB
    + importlib_metadata                    6.7.0  hd8ed1ab_0              conda-forge/noarch         9kB
    + importlib_resources                  5.12.0  pyhd8ed1ab_0            conda-forge/noarch        31kB

Look at the sizes of the pkgs - lemme try try not specifying importlib_resources in our env

@valeriupredoi

valeriupredoi commented Jun 22, 2023

Copy link
Copy Markdown
Contributor

boom! Problem solved - by good old conda-forge packaging 😁 If you want to you can add importlib_metadata as direct requirement for us, but I'd not really bother since that's coming from scipy via iris 👍 Oh and maybe someone should tell Python core devs that importlib.metadata is broke for Python 3.9, but I've had enough conflict resolution done today (elsewhere) 😆

@bouweandela

Copy link
Copy Markdown
Member Author

OK, but the point of this pull request was to use the Python standard library instead of the package that backports that to older Python versions. I'll have a look if I can get it to work for Python 3.9 when I have some time.

@valeriupredoi

Copy link
Copy Markdown
Contributor

Why are you keen on using the standard library - as you can see from here, the c-f package is more stable, rather, I thought the idea was to retire pkg_resources 🍺

@zklaus

zklaus commented Jun 28, 2023

Copy link
Copy Markdown

I'd like to explain a bit the ecosystem. Pypi decided that - and _ are the same thing, i.e. in URLs, and when installing stuff with Pip you can always use either. Conda-forge does not work like that. If you want to support both formats, you need to provide two packages, which is relatively frequently done with multi-output recipes, i.e. a single recipe producing multiple packages. In that case, it usually creates one of the two packages as a dummy that depends on the other one. Here, the recipe generates the real package as importlib-metadata and a dummy package importlib_metadata that depends on the former.

So when you depend on importlib_metadata you automatically also depend on importlib-metadata, but these are not distinct packages.

ah then I chased a red herring then fish - still, for Python 3.9 and only for Python 3.9 there are two importlib-/_resources:

How did you come to this conclusion? The conda-forge packages are noarch, so there aren't different packages for different Python versions.

Comment thread esmvalcore/_main.py Outdated
self.recipes = Recipes()
self._extra_packages = {}
if not list(iter_entry_points('esmvaltool_commands')):
if not list(entry_points(group='esmvaltool_commands')):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably avoid calling entry_points twice. Do we really need to turn this into a list? Or would if not entry_points(... work?

@zklaus

zklaus commented Jun 28, 2023

Copy link
Copy Markdown

so importlib_metadata is an old package that was decomissioned by the maintainer in January, see https://anaconda.org/anaconda/importlib_metadata - [...]

You are looking in the wrong place. We don't care about Anaconda packages; the corresponding Conda-forge package is alive and well at https://anaconda.org/conda-forge/importlib_metadata.

@zklaus

zklaus commented Jun 29, 2023

Copy link
Copy Markdown

To record some insight we generated at the workshop yesterday: In Python 3.9, entry_points() does not have a group keyword and instead just returns all entry points as a Python dict keyed by the entry point groups. From Python 3.10 on, entry_points is more powerful and can do some filtering. It also does not return a dict, but a specialized EntryPoints object.

Both dict in Python 3.9 and EntryPoints in 3.10+ support [] access, so we could just use entry_points()["emsvaltool_commands"], which will return all registered esmvaltool commands and raise a KeyError if there none.

The downside is that from 3.10 on, this usage is deprecated and will generate a DeprecationWarning; I could not find out when it will actually be removed.

@bouweandela bouweandela force-pushed the use-importlib-metadata branch from b4fd2fe to a97befd Compare July 3, 2023 10:02
@zklaus

zklaus commented Jul 3, 2023

Copy link
Copy Markdown

Should we really be removing importlib_resources? Note that this is a different thing than importlib_metadata. While importlib_metadata is used to access entry points, distribution package names, etc., importlib_resources is used to access resources, such as binary and text files like recipes in the distribution. Confusingly, both functionalities were provided by pkg_resources.

We should be using importlib_resources (also in the standard lib as importlib.resources) to load packaged recipes and configuration files, though we may not be doing that consequently at the moment. Basically, all uses of __file__ should be replaced with some use of importlib resources. That is a different discussion, but I point it out here to ask whether we really want to remove the dependency on importlib_resources in this PR.

@zklaus

zklaus commented Jul 3, 2023

Copy link
Copy Markdown

PS: Seems the only place where this is used is at

if sys.version_info[:2] >= (3, 9):
, so if we consider Python 3.8 support to be dropped, we can remove the dependency, but then should also remove the special import code there, probably both together in a separate PR.

@bouweandela

bouweandela commented Jul 3, 2023

Copy link
Copy Markdown
Member Author

Yes, I forgot to remove that code when dropping support for Python 3.8 in #2053 and then I spotted the old dependencies, thought I might as well replace them with what we currently need. Did a grep through the code for any uses, but apparently missed the use in esmvalcore/config/_config.py.

@valeriupredoi

Copy link
Copy Markdown
Contributor

python=3.8 went to the fishes a while back, Klaus 🐟

@zklaus zklaus left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the github actions trigger I think this is ok.

As a side note, we might want to change the name for the entry points group to esmvaltool.commands. That would be more in line with the specification and prepare us for possible future entry points esmvaltool.recipes and esmvaltool.diagnostics.

Comment thread .github/workflows/run-tests.yml Outdated
push:
branches:
- main
- use-importlib-metadata

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be removed prior to merging.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done! 🍺

Comment thread .github/workflows/run-tests.yml Outdated
@zklaus zklaus merged commit e528f96 into main Jul 6, 2023
@zklaus zklaus deleted the use-importlib-metadata branch July 6, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

installation Installation problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pkg_resources is deprecated

3 participants