Update collection loader for Python 3.10 #76225

s-hertel · 2021-11-04T14:20:48Z

SUMMARY

Fixes #74660

Implement find_spec and exec_module to remove reliance on deprecated methods find_module and load_module.

ISSUE TYPE

Feature Pull Request

…methods in the collection loader ci_complete

lib/ansible/utils/collection_loader/_collection_finder.py

nitzmahone

Added comments to sivel's; the other piece of this that looks plausible is actually just getting rid of ansible-test's copy of the collection loader. We assumed it wouldn't be very feasible to have a combo 2.x/3.x loader, but it kinda looks like it might be, so if this all works, we should probably (either as part of this PR or right after) get rid of the second copy altogether.

lib/ansible/utils/collection_loader/_collection_finder.py

Remove extra sys.modules handling Use default module initialization by returning None from loader.create_module Refactor ci_complete

ci_complete

s-hertel · 2021-11-11T16:56:34Z

lib/ansible/utils/collection_loader/_collection_finder.py

+try:
+    from importlib.util import spec_from_loader
+except ImportError:
+    pass


It shouldn't ever happen, but maybe I should set spec_from_loader to None here, and raise a ValueError in find_spec if it's None.

I'm actually a little surprised pylint doesn't complain about the possibly undefined name here, but if it genuinely doesn't, we're basically getting the behavior we want for free the way the code is now (a NameError on spec_from_loader if someone calls find_spec, which is probably more useful to anyone that cares than the extra effort required for more explicit error handling. So I'd vote for "leave as is" if pylint's not complaining.

mattclay · 2021-11-11T18:57:29Z

@s-hertel The Python version filter in ansible-test should be updated to include the collection loader in the tests for target-only Python versions. That can be done by adding lib/ansible/utils/collection_loader to the list of paths here:

ansible/test/lib/ansible_test/_internal/commands/sanity/__init__.py

Line 774 in fe77bc9

    
           def filter_remote_targets(targets):  # type: (t.List[TestTarget]) -> t.List[TestTarget]

The main effect of that change will be to include the collection loader in the compile tests for all Python versions.

samccann · 2021-11-11T21:26:36Z

does it warrant a changelog fragment??

s-hertel · 2021-11-12T23:22:26Z

@samccann Probably! Added one.

samccann

changelog fragment lgtm :-)

nitzmahone

Looking good! A few extraneous lines, a couple of recommended explicit returns, and a couple of corner cases to talk through, but I think this is pretty much ready to go.

nitzmahone · 2021-11-18T01:20:33Z

lib/ansible/utils/collection_loader/_collection_finder.py

+try:
+    from importlib.util import spec_from_loader
+except ImportError:
+    pass


I'm actually a little surprised pylint doesn't complain about the possibly undefined name here, but if it genuinely doesn't, we're basically getting the behavior we want for free the way the code is now (a NameError on spec_from_loader if someone calls find_spec, which is probably more useful to anyone that cares than the extra effort required for more explicit error handling. So I'd vote for "leave as is" if pylint's not complaining.

lib/ansible/utils/collection_loader/_collection_finder.py

Remove get_filename short-circuit exec_module if it's a redirect ci_complete

nitzmahone

Looks great- thanks!

s-hertel · 2021-12-01T17:47:30Z

Thanks for all the reviews!

webknjaz

I forgot to publish this draft review. So posting it post-merge for history. In case somebody will want to refactor these things.

webknjaz · 2021-11-30T20:56:20Z

lib/ansible/utils/collection_loader/_collection_finder.py

+        if finder is not None:
+            if toplevel_pkg == 'ansible_collections':
+                return finder.find_spec(fullname, path=[self._pathctx])
            else:
-                # call py2's internal loader
-                return pkgutil.ImpImporter(self._pathctx).find_module(fullname)
+                return finder.find_spec(fullname)
+        else:
+            return None


nitpick: I think this could have less nesting.

Suggested change

if finder is not None:

if toplevel_pkg == 'ansible_collections':

return finder.find_spec(fullname, path=[self._pathctx])

else:

# call py2's internal loader

return pkgutil.ImpImporter(self._pathctx).find_module(fullname)

return finder.find_spec(fullname)

else:

return None

if finder is None:

return None

if toplevel_pkg == 'ansible_collections':

return finder.find_spec(fullname, path=[self._pathctx])

return finder.find_spec(fullname)

webknjaz · 2021-11-30T20:59:01Z

lib/ansible/utils/collection_loader/_collection_finder.py

+        if loader:
+            spec = spec_from_loader(fullname, loader)
+            if spec is not None and hasattr(loader, '_subpackage_search_paths'):
+                spec.submodule_search_locations = loader._subpackage_search_paths
+            return spec
+        else:
+            return None


nit: this is a good place to use a "guard expression"

Suggested change

if loader:

spec = spec_from_loader(fullname, loader)

if spec is not None and hasattr(loader, '_subpackage_search_paths'):

spec.submodule_search_locations = loader._subpackage_search_paths

return spec

else:

return None

if not loader:

return None

spec = spec_from_loader(fullname, loader)

if spec is not None and hasattr(loader, '_subpackage_search_paths'):

spec.submodule_search_locations = loader._subpackage_search_paths

return spec

felixfontein · 2021-12-02T09:29:52Z

This PR seems to cause some problems in some community.general unit tests for Python 3.7 to 3.9: https://dev.azure.com/ansible/community.general/_build/results?buildId=30854&view=logs&j=83b878b7-b763-5cdf-bf1e-fd8bdd476fa6&t=450760e6-4f51-5fd0-aaa0-1d38d376b06c

felixfontein · 2021-12-02T09:30:46Z

_______________ ERROR collecting cloud/linode/test_linode_v4.py ________________
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:2194: in _handle_ns
02:10     loader = importer.find_spec(packageName).loader
02:10 E   AttributeError: 'NoneType' object has no attribute 'loader'
02:10 
02:10 During handling of the above exception, another exception occurred:
02:10 tests/unit/plugins/modules/cloud/linode/test_linode_v4.py:10: in <module>
02:10     linode_apiv4 = pytest.importorskip('linode_api4')
02:10 /usr/local/lib/python3.9/dist-packages/linode_api4/__init__.py:3: in <module>
02:10     from linode_api4.linode_client import LinodeClient
02:10 /usr/local/lib/python3.9/dist-packages/linode_api4/linode_client.py:7: in <module>
02:10     import pkg_resources
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:3243: in <module>
02:10     def _initialize_master_working_set():
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:3226: in _call_aside
02:10     f(*args, **kwargs)
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:3268: in _initialize_master_working_set
02:10     tuple(
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:3269: in <genexpr>
02:10     dist.activate(replace=False)
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:2783: in activate
02:10     declare_namespace(pkg)
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:2282: in declare_namespace
02:10     _handle_ns(packageName, path_item)
02:10 /usr/local/lib/python3.9/dist-packages/pkg_resources/__init__.py:2199: in _handle_ns
02:10     loader = importer.find_module(packageName)
02:10 /tmp/ansible-test-u4ejf2o8/ansible/utils/collection_loader/_collection_finder.py:303: in find_module
02:10     return finder.find_module(fullname, path=[self._pathctx])
02:10 E   TypeError: _find_module_shim() got an unexpected keyword argument 'path'

s-hertel · 2021-12-02T18:42:09Z

@felixfontein I'm looking at this but don't have a fix yet. Thanks for spotting the issue. Running the tests locally fails during getting the metadata, so I'm not sure if my pokings around are going in the right direction (not sure if that's an additional bug or if I'm just running the tests wrong - do you get the same error testing tests/unit/plugins/modules/cloud/linode/test_linode_v4.py locally?). Do you know if there is a way to open a PR against community.general to run CI against an open ansible/ansible PR?

felixfontein · 2021-12-02T21:03:55Z

@s-hertel there isn't a "ready to use" way to do this; what I usually do is a) kick out almost everything in https://github.com/ansible-collections/community.general/blob/main/.azure-pipelines/azure-pipelines.yml so that only the tests I'm interested are running, and b) modify https://github.com/ansible-collections/community.general/blob/main/tests/utils/shippable/shippable.sh#L60 to install another branch from another user.

s-hertel · 2021-12-02T21:16:29Z

@felixfontein Thanks much, good to know 😄 I have a local reproducer now (I was running the test without --docker).

Implement find_spec and exec_module to remove reliance on deprecated …

63a23c7

…methods in the collection loader ci_complete

sivel requested review from nitzmahone and sivel November 4, 2021 14:30

bcoca removed the needs_triage Needs a first human triage before being processed. label Nov 4, 2021

sivel reviewed Nov 5, 2021

View reviewed changes

lib/ansible/utils/collection_loader/_collection_finder.py Outdated Show resolved Hide resolved

sivel reviewed Nov 5, 2021

View reviewed changes

lib/ansible/utils/collection_loader/_collection_finder.py Show resolved Hide resolved

sivel reviewed Nov 5, 2021

View reviewed changes

lib/ansible/utils/collection_loader/_collection_finder.py Outdated Show resolved Hide resolved

sivel reviewed Nov 5, 2021

View reviewed changes

lib/ansible/utils/collection_loader/_collection_finder.py Outdated Show resolved Hide resolved

nitzmahone requested changes Nov 5, 2021

View reviewed changes

webknjaz reviewed Nov 6, 2021

View reviewed changes

lib/ansible/utils/collection_loader/_collection_finder.py Outdated Show resolved Hide resolved

lib/ansible/utils/collection_loader/_collection_finder.py Outdated Show resolved Hide resolved

s-hertel force-pushed the py3ify_collection_finder branch from ee0e4df to 26abcf6 Compare November 8, 2021 22:07

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Nov 8, 2021

Move module execution to exec_module

1abaebe

Remove extra sys.modules handling Use default module initialization by returning None from loader.create_module Refactor ci_complete

s-hertel force-pushed the py3ify_collection_finder branch from 26abcf6 to 1abaebe Compare November 8, 2021 22:23

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Nov 8, 2021

s-hertel force-pushed the py3ify_collection_finder branch from 8f8171e to 0ece0af Compare November 8, 2021 23:16

This comment has been minimized.

Sign in to view

Remove ansible-test's copy of the collection loader

754796a

ci_complete

s-hertel force-pushed the py3ify_collection_finder branch from 0ece0af to 754796a Compare November 9, 2021 12:05

s-hertel marked this pull request as ready for review November 9, 2021 12:06

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Nov 9, 2021

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Nov 10, 2021

s-hertel commented Nov 11, 2021

View reviewed changes

s-hertel added 2 commits November 12, 2021 23:17

Include collection_loader in target paths for 'compile' sanity test

2a873c0

add changelog

15b5c73

samccann approved these changes Nov 15, 2021

View reviewed changes

nitzmahone requested changes Nov 18, 2021

View reviewed changes

Add "return None" instead of doing it implicitly

0a5ea13

Remove get_filename short-circuit exec_module if it's a redirect ci_complete

s-hertel force-pushed the py3ify_collection_finder branch from e3b271c to 0a5ea13 Compare November 30, 2021 20:40

nitzmahone approved these changes Dec 1, 2021

View reviewed changes

nitzmahone merged commit b50f16d into ansible:devel Dec 1, 2021

webknjaz reviewed Dec 2, 2021

View reviewed changes

s-hertel mentioned this pull request Dec 2, 2021

check finder type before passing path #76448

Merged

ansible locked and limited conversation to collaborators Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update collection loader for Python 3.10 #76225

Update collection loader for Python 3.10 #76225

s-hertel commented Nov 4, 2021

nitzmahone left a comment

This comment has been minimized.

s-hertel Nov 11, 2021

nitzmahone Nov 18, 2021

mattclay commented Nov 11, 2021

samccann commented Nov 11, 2021

s-hertel commented Nov 12, 2021

samccann left a comment

nitzmahone left a comment

nitzmahone Nov 18, 2021

nitzmahone left a comment

s-hertel commented Dec 1, 2021

webknjaz left a comment

webknjaz Nov 30, 2021

webknjaz Nov 30, 2021

felixfontein commented Dec 2, 2021

felixfontein commented Dec 2, 2021

s-hertel commented Dec 2, 2021 •

edited

felixfontein commented Dec 2, 2021

s-hertel commented Dec 2, 2021

Update collection loader for Python 3.10 #76225

Update collection loader for Python 3.10 #76225

Conversation

s-hertel commented Nov 4, 2021

SUMMARY

ISSUE TYPE

nitzmahone left a comment

Choose a reason for hiding this comment

This comment has been minimized.

s-hertel Nov 11, 2021

Choose a reason for hiding this comment

nitzmahone Nov 18, 2021

Choose a reason for hiding this comment

mattclay commented Nov 11, 2021

samccann commented Nov 11, 2021

s-hertel commented Nov 12, 2021

samccann left a comment

Choose a reason for hiding this comment

nitzmahone left a comment

Choose a reason for hiding this comment

nitzmahone Nov 18, 2021

Choose a reason for hiding this comment

nitzmahone left a comment

Choose a reason for hiding this comment

s-hertel commented Dec 1, 2021

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz Nov 30, 2021

Choose a reason for hiding this comment

webknjaz Nov 30, 2021

Choose a reason for hiding this comment

felixfontein commented Dec 2, 2021

felixfontein commented Dec 2, 2021

s-hertel commented Dec 2, 2021 • edited

felixfontein commented Dec 2, 2021

s-hertel commented Dec 2, 2021

s-hertel commented Dec 2, 2021 •

edited