Use importlib instead of pkg_resources for determining namespace packages

[DanielNoord]

Steps

• For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• Write a good description on what the PR does.

Description

Work in progress.

I don't think we can fully remove pkg_resources just yet. There are two test cases that fail without is_old_setuptools_namespace_package.
However, I think those might be non-sensical as that interfere with _namespace_packages directly instead of actually registering any packages. I tested this and this works without is_old_setuptools_namespace_package for normally registered setuptools namespace package's.

I also ran into a weird bug with find_spec that I have filed to see if I'm doing anything wrong.

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

Ref #1103

[Pierre-Sassoulas]

LGTM. Let's see what comes of the bug you opened.

[DanielNoord]

This was actually quite difficult to get right and I'm not sure I'm quite there yet.

I'm especially wondering if the handling of the ImportError at the end is correct. It is necessary for pylint as the tests directory of pylint is a namespace package that then gets "specced" on another tests directory with a __init__ file. In my case this was astroid. Then an ImportError occurs when importing.
So the ImportError seems to actually indicate a namespace package.

I also wonder if the is_old_setuptools_namespace_package function is necessary. I think the return statement at the end in the normal function does the same and this function is only needed for our tests to pass, but I'm not 100% sure. I tested it and it worked without it, but we might want to gather feedback?

[Pierre-Sassoulas]

I have a hard time reviewing this, this is a delicate part of the code as there was a lot of issue with namespace package and __init__.pyin the past. On the other hand I would love to reduce astroid start up time 😄

we might want to gather feedback?

Yes, but how would we do that ? Asking user to install this branch ? Temporary more primer tests in pylint ?

Also, should we keep this in astroid 2.10 ? I'd like to release pylint 2.13, maybe there's some issue that are not strictly necessary. To be honest I'm afraid of merging this so I let it go stale but having 300+ issues in pylint 2.13 milestone is not ideal.

[DanielNoord]

I have a hard time reviewing this, this is a delicate part of the code as there was a lot of issue with namespace package and __init__.pyin the past. On the other hand I would love to reduce astroid start up time 😄

I'm wondering if this is actually used that often in pylint. The old code seems to only look for pkg_resources namespace packages. I haven't looked at the internals of pkg_resources but I wonder if normal namespace packages (ie., packages without an __init__.py also show up in pkg_resources._namespace_packages.

If not, then this is just a more elaborate function which just hides the import of pkg_resources so it is only performed when necessary 😄

we might want to gather feedback?

Yes, but how would we do that ? Asking user to install this branch ? Temporary more primer tests in pylint ?

Also, should we keep this in astroid 2.10 ? I'd like to release pylint 2.13, maybe there's some issue that are not strictly necessary. To be honest I'm afraid of merging this so I let it go stale but having 300+ issues in pylint 2.13 milestone is not ideal.

I mean: I would think it makes sense. It would be a nice performance increase.

[Pierre-Sassoulas]

Let's merge then :) Do you think it's ready ?

[DanielNoord]

Let's merge then :) Do you think it's ready ?

I think so. I only worry a little about the handling of the ImportError if that's correct. But I think it is.

[cdce8p]

This PR seems to cause a major performance regression. I only have the Github Action numbers for Home-Assistant to compare with, but there I'm seeing a 35% slower test run (with 2 jobs). Furthermore, I noticed unexpected debug log messages which might suggest packages are actually imported at runtime instead of only analyzed.

Some examples:

asyncio - DEBUG - Using selector: EpollSelector
matplotlib - DEBUG - matplotlib data path: /__w/ha-core/ha-core/venv/lib/python3.9/site-packages/matplotlib/mpl-data
matplotlib - DEBUG - CONFIGDIR=/github/home/.config/matplotlib
matplotlib - DEBUG - interactive is False
matplotlib - DEBUG - platform is linux
...
graphviz._tools - DEBUG - deprecate positional args: graphviz.backend.piping.pipe(['renderer', 'formatter', 'quiet'])
graphviz._tools - DEBUG - deprecate positional args: graphviz.backend.rendering.render(['renderer', 'formatter', 'quiet'])
graphviz._tools - DEBUG - deprecate positional args: graphviz.backend.unflattening.unflatten(['stagger', 'fanout', 'chain', 'encoding'])
graphviz._tools - DEBUG - deprecate positional args: graphviz.backend.viewing.view(['quiet'])
...

[DanielNoord]

That's definetly not good. Sorry about that.

My first thought is that it might be due to spec.loader.get_source(spec.origin). I didn't think it would actually import the module based on the documentation, but seeing as it raises ImportError it might actually load the module and then get the source.
That would be rather annoying though as we will then need to start re-importing pkg_resources in almost all cases.
Potential fix would be to replace L65-L71 with a call to _is_old_setuptools_namespace_package. However, that would be reached so often that I don't think we can say that we no longer "depend on "/import pkg_resources.

[Pierre-Sassoulas]

Maybe we can check if pkg_ressource is already a value in global instead of trying to import every-time ?