Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated load_module with exec_module #498

Merged
merged 4 commits into from Oct 8, 2021
Merged

Replace deprecated load_module with exec_module #498

merged 4 commits into from Oct 8, 2021

Conversation

FollowTheProcess
Copy link
Collaborator

Closes #494

This was a bit of a nightmare! 馃槵 this implementation is by no means perfect and it took a great deal of trial and error as well as at least a few days off my lifespan!

A brief summary of what I've done and how I got here (not all of this is in the PR commit history):

  • The deprecation warning attached to load_module suggests replacing with exec_module. However, nox needs the module to be passed around and exec_module returns None so this was a non-starter.
  • I then tried to replace it with a call to importlib.import_module as this will return the ModuleType. This appeared to work when using nox at the command line but every test that uses one of the demo noxfiles in the tests/resources/ kept failing with a ModuleNotFound error. I tried various things to fix this such as making the resources dir a valid package, manually changing directories in the tests, trying to resolve relative to absolute imports etc. All to no avail!
  • Then I more or less arrived at the current implementation by roughly following this: https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly.

This still gave me some grief as mypy wouldn't recognise the exec_module method despite its use in the exact same way as the example in the link, so I had to # type ignore that and I had some difficulty with ensuring coverage. You'll notice a single # pragma: no cover for one of the guard statements as I just couldn't figure out how to mock it out properly for tests.

I think if we could get importlib.import_module to work here this would be by far the cleanest solution, but as I said I couldn't get it to pass the tests, it kept importing the project's noxfile, not the ones under tests/resources which was very weird!

@FollowTheProcess FollowTheProcess changed the title Replace load_module Pending deprecation: replace load_module Oct 8, 2021
@theacodes theacodes changed the title Pending deprecation: replace load_module Replace deprecated load_module with exec_module Oct 8, 2021
Copy link
Collaborator

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for doing this.

@theacodes theacodes merged commit e9f7f03 into wntrblm:main Oct 8, 2021
@FollowTheProcess FollowTheProcess deleted the load-module-deprecation branch October 9, 2021 08:59
fkiraly added a commit to sktime/skbase that referenced this pull request Oct 4, 2023
`load_module` is deprecated and will be replaced by `exec_module` in
python 3.12, this PR addresses that.

The fix is highly nontrivial - the crucial line was
`imported_mod = module.load_module()` for a `module: SourceFileLoader`,

which had to be replaced by the equivalent four (!) lines

```python
spec = importlib.util.spec_from_file_location(module.name, module.path)
imported_mod = importlib.util.module_from_spec(spec)

# these two are also crucial due to the side effects
loader = spec.loader
loader.exec_module(imported_mod)
```

The deprecation message raised by `load_module` is highly unhelpful, and I had to reverse engineer this from a very helpful PR in `nox`, wntrblm/nox#498
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

DeprecationWarning: the load_module() method is deprecated and slated for removal in Python 3.12
2 participants