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

PICARD-2354: DeprecationWarning: the imp module is deprecated in favour of importlib #2134

Closed
wants to merge 10 commits into from

Conversation

skelly37
Copy link
Contributor

Summary

The imp module will be removed in Python 3.12 so we have to switch to importlib

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

imp is used only in pluginmanager.py in 2 places, so it doesn't change much of the codebase.

Problem

Solution

I've found some way to replace it in importlib examples but I'm not sure if they're the best way to achieve this.

Action

@skelly37
Copy link
Contributor Author

@skelly37
Copy link
Contributor Author

I'm clueless, I've managed to create a module and to pass it further as the same thing, yet the tests still fail

importlib:

>>> d = importlib.machinery.PathFinder().find_spec("picard", ["/data/it/picard"])
>>> d
ModuleSpec(name='picard', loader=<_frozen_importlib_external.SourceFileLoader object at 0x7fd4e9d9a9e0>, origin='/data/it/picard/picard/__init__.py', submodule_search_locations=['/data/it/picard/picard'])
>>> a = importlib.util.module_from_spec(d)
>>> a
<module 'picard' from '/data/it/picard/picard/__init__.py'>

imp:

>>> i = imp.find_module("picard", ["/data/it/picard"])
>>> i
(None, '/data/it/picard/picard', ('', '', 5))
>>> p = imp.load_module("picard", *i)
>>> p
<module 'picard' from '/data/it/picard/picard/__init__.py'>

@phw phw added this to the 3.0 milestone Sep 27, 2022
@phw
Copy link
Member

phw commented Dec 11, 2022

I made some progress here with fa36ca4 . What was missing was actually executing the module. The newer interfaces of importlib separate loading the module and executing it. Without this all the metadata was not available and the tests failed.

This separation, while not used by Picard currently, actually might help a lot in a future rework of the plugin system, as one of the major downsides of the plugin system right now is that all plugins get executed, even if disabled.

Anyway, the change is not sufficient to make the tests pass. What is still missing is that import picard.plugins.theplugin does not work yet. I'm not sure how to solve this.

Maybe we really need to go another route and we implement plugin loading by doing a custom importer implementation. That probably means implementing a meta path finder and / or a path entry finder as explained in https://docs.python.org/3/library/importlib.html#setting-up-an-importer . I don't yet fully grasp the details here, but currently this looks to me like it would allow us to tackle the issues we still have. Maybe relying more on the capabilities of the importlib API might even simplify the code for loading we currently have.

@phw
Copy link
Member

phw commented Jun 12, 2023

We will look into this after the PyQt6 changes have been merged. Maybe we need to rethink plugin handling and reimplement parts of it.

@zas
Copy link
Collaborator

zas commented Jun 12, 2023

We will look into this after the PyQt6 changes have been merged. Maybe we need to rethink plugin handling and reimplement parts of it.

It seems we'll have more to do, since zipimport also has deprecated methods we are using:

zipimporter.find_module() is deprecated and slated for removal in Python 3.12; use find_spec() instead

if not zip_importer.find_module(name):

https://github.com/python/cpython/blob/171aa086f27e581bfcf2288d5afd0167480ef3cb/Lib/zipimport.py#L146-L160

@phw
Copy link
Member

phw commented Jun 12, 2023

@zas yes, indeed. We already have https://tickets.metabrainz.org/browse/PICARD-2355 for this.

Maybe it will be easier to implement a fresh plugins3 without dealing with the legacy code. @rdswift already had collected some ideas quite a while ago, and I have some specific ideas.I'll see I can write this down so we can start a discussion.

@rdswift
Copy link
Collaborator

rdswift commented Jun 12, 2023

Maybe it will be easier to implement a fresh plugins3 without dealing with the legacy code. @rdswift already had collected some ideas quite a while ago, and I have some specific ideas.I'll see I can write this down so we can start a discussion

The notes from my initial discussion can be found at https://github.com/rdswift/picard-plugins/wiki/Picard-Plugins-System-Proposal if you're interested, and the related ticket PICARD-1861.

@phw
Copy link
Member

phw commented Sep 9, 2023

Closing, replaced by #2307

@phw phw closed this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants