-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
Update collection loader for Python 3.10 #76225
Conversation
…methods in the collection loader ci_complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ee0e4df
to
26abcf6
Compare
Remove extra sys.modules handling Use default module initialization by returning None from loader.create_module Refactor ci_complete
26abcf6
to
1abaebe
Compare
8f8171e
to
0ece0af
Compare
This comment has been minimized.
This comment has been minimized.
0ece0af
to
754796a
Compare
try: | ||
from importlib.util import spec_from_loader | ||
except ImportError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@s-hertel The Python version filter in
The main effect of that change will be to include the collection loader in the |
does it warrant a changelog fragment?? |
@samccann Probably! Added one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog fragment lgtm :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
try: | ||
from importlib.util import spec_from_loader | ||
except ImportError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Remove get_filename short-circuit exec_module if it's a redirect ci_complete
e3b271c
to
0a5ea13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great- thanks!
Thanks for all the reviews! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to publish this draft review. So posting it post-merge for history. In case somebody will want to refactor these things.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I think this could have less nesting.
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) |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is a good place to use a "guard expression"
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 |
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 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? |
@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. |
@felixfontein Thanks much, good to know 😄 I have a local reproducer now (I was running the test without --docker). |
SUMMARY
Fixes #74660
Implement find_spec and exec_module to remove reliance on deprecated methods find_module and load_module.
ISSUE TYPE