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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated import of imp module #3350

Merged
merged 1 commit into from Feb 10, 2020
Merged

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Feb 10, 2020

Fixes gh-3306

Using imp is deprecated. Add an alternative incantation from importlib which was added in Python 3.3

@scoder
Copy link
Contributor

scoder commented Feb 10, 2020

Thanks

@scoder scoder merged commit 9355dc5 into cython:master Feb 10, 2020
@scoder scoder added this to the 0.29.15 milestone Feb 10, 2020
@realead
Copy link
Contributor

realead commented Feb 10, 2020

There was https://bugs.python.org/issue24748, it is probably also important in this context.

The official version of load_dynamic is:

    def load_dynamic(name, path, file=None):
        """**DEPRECATED**
        Load an extension module.
        """
        import importlib.machinery
        loader = importlib.machinery.ExtensionFileLoader(name, path)

        # Issue #24748: Skip the sys.modules check in _load_module_shim;
        # always load new extension
        spec = importlib.machinery.ModuleSpec(
            name=name, loader=loader, origin=path)
        return _load(spec)

In pyximport there is also imp imported, i.e. https://github.com/cython/cython/blob/master/pyximport/pyximport.py#L51

However, I'm not sure the cure is not worse than the disease: What is the point in copying the code from imp and then fixing all the copies once a new bug was fixed in imp?

@mattip
Copy link
Contributor Author

mattip commented Feb 11, 2020

PEP 489 states "All changed loaders (BuiltinImporter and ExtensionFileLoader) will remain backwards-compatible; the load_module method will be replaced by a shim." so I think this change is safe until those shims will be removed by a future PEP. It could be improved to support PEP 489 multi-phase initialization if needed.

The python issue you point to has to do with loading a module when that module already has been loaded. This is not possible here since the module is newly-compiled.

In pyximport there is also imp imported, i.e. https://github.com/cython/cython/blob/master/pyximport/pyximport.py#L51

Interesting, cython is indeed huge. I guess that should be changed too.

@realead
Copy link
Contributor

realead commented Feb 11, 2020

The python issue you point to has to do with loading a module when that module already has been loaded. This is not possible here since the module is newly-compiled.

That is true. However, in the context of pyximport, which supports reloading, this probably should be taken into consideration. Having different implementations of load_dynamic might become quite confusing.

@scoder scoder modified the milestones: 0.29.15, 0.29.16 Feb 12, 2020
scoder pushed a commit that referenced this pull request Feb 12, 2020
@mattip mattip deleted the imp_to_importlib branch May 30, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeprecationWarning: the imp module is deprecated in favour of importlib
3 participants