PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

phw · 2023-09-09T14:29:12Z

Summary

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

Problem

JIRA ticket (optional): PICARD-2751, PICARD-2354, PICARD-2355

Picard's plugin loading system must be updated to avoid long deprecated and inf Python 3.12 finally removed APIs of the importlib and zipimport modules.

This PR replaces #2134

Solution

At the core the change is to use find_spec in combination with exec_module to load the modules. A large benefit is that both the normal module loading via importlib as well as the zipimporter can now use the exactly same API.

Unfortunately for zipimporter we need to maintain the legacy loading code with find_module + load_module for compatibility with Python < 3.10.

This PR maintains full compatibility with the existing plugin systems. Overall there is likely a lot that we could further refactor and trim down, but that is something to be reserved for Picard 3.0.

To properly test the result this PR also enables CI testing with Python 3.12.0rc1

test/test_plugins.py

Avoids use of deprecated and in Python 3.12 removed API

hboetes · 2023-09-09T15:21:00Z

FYI I just tried your PR, and it works, but I get these errors.

E: 17:15:51,086 config.__init__:424: Option setting/rsgain_command already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:353: in ReplayGain2OptionsPage
        TextOption("setting", "rsgain_command", "rsgain"),
E: 17:15:51,088 config.__init__:424: Option setting/album_tags already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:354: in ReplayGain2OptionsPage
        BoolOption("setting", "album_tags", True),
E: 17:15:51,090 config.__init__:424: Option setting/true_peak already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:355: in ReplayGain2OptionsPage
        BoolOption("setting", "true_peak", False),
E: 17:15:51,091 config.__init__:424: Option setting/reference_loudness already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:356: in ReplayGain2OptionsPage
        BoolOption("setting", "reference_loudness", False),
E: 17:15:51,093 config.__init__:424: Option setting/target_loudness already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:357: in ReplayGain2OptionsPage
        IntOption("setting", "target_loudness", -18),
E: 17:15:51,094 config.__init__:424: Option setting/clip_mode already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:358: in ReplayGain2OptionsPage
        IntOption("setting", "clip_mode", CLIP_MODE_POSITIVE),
E: 17:15:51,096 config.__init__:424: Option setting/max_peak already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:359: in ReplayGain2OptionsPage
        IntOption("setting", "max_peak", 0),
E: 17:15:51,097 config.__init__:424: Option setting/opus_mode already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:360: in ReplayGain2OptionsPage
        IntOption("setting", "opus_mode", OPUS_MODE_STANDARD),
E: 17:15:51,099 config.__init__:424: Option setting/opus_m23 already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:361: in ReplayGain2OptionsPage
        BoolOption("setting", "opus_m23", False)
Time: 0h:00m:06s

hboetes · 2023-09-09T15:28:39Z

And it crashes upon changing the configuration.

phw · 2023-09-09T15:35:43Z

@hboetes Thanks for the feedback. The log output is quite interesting. I debugged this a bit. The plugin code gets run twice at exactly the line https://github.com/metabrainz/picard/pull/2307/files#diff-c2f162faf440349fc5556185e9cd8b4b3cf0f35235eb561eb3fd7261651a2fbfR356 . That means the spec.loader.exec_module(plugin_module) call is enough for this to happen. Unsure how to avoid this.

And it crashes upon changing the configuration.

Can you elaborate? Any error output on the terminal?

hboetes · 2023-09-09T15:40:54Z

Yes, this actually happened after pressing the settings button, the first time it complains, the second time it crashes.

Edit: That's a bug in the replaygain plugin, after disabling that by hacking in the config that problem is gone.

Traceback (most recent call last):
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/mainwindow.py", line 1313, in show_options
    options_dialog = OptionsDialog.show_instance(page, self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 156, in show_instance
    instance = cls.get_instance(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 150, in get_instance
    cls._instance = cls(*args, **kwargs)
                    ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 168, in __init__
    self.add_pages(None, default_page, self.ui.pages_tree)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 113, in add_pages
    self.add_pages(page.NAME, default_page, item)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 104, in add_pages
    for foo, bar, page in sorted(pages):
                          ^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'ReplayGain2OptionsPage' and 'ReplayGain2OptionsPage'
Traceback (most recent call last):
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/mainwindow.py", line 1313, in show_options
    options_dialog = OptionsDialog.show_instance(page, self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 156, in show_instance
    instance = cls.get_instance(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 150, in get_instance
    cls._instance = cls(*args, **kwargs)
                    ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 168, in __init__
    self.add_pages(None, default_page, self.ui.pages_tree)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 113, in add_pages
    self.add_pages(page.NAME, default_page, item)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 104, in add_pages
    for foo, bar, page in sorted(pages):
                          ^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'ReplayGain2OptionsPage' and 'ReplayGain2OptionsPage'
[1]    2000396 IOT instruction  picard

hboetes · 2023-09-09T17:38:43Z

For the rest it seems to be working. 👍

zas

It looks good to me, I tested and didn't find any issue, good job!

picard/pluginmanager.py

…y twice

phw · 2023-09-10T11:16:18Z

I have fixed the double execution in 4db506c .

Essentially this happened for all plugins that themselves have relative imports. When running spec.loader.exec_module the module is not yet sys.modules. So if the module does a relative import on itself the module gets imported again. This change feels kind of like a hack, but also the existing behavior using the APIs seems slightly off. I'm not fully sure, but the solution / workaround works.

In the last commit I also changed a bit how plugins get loaded if they exist in multiple directories. The original implementation first tried to load from system plugin dir, but if it also existed in the user plugin dir it "unloaded" the previous plugin and loaded the new one. Now because running exec_module causes the module body to run this could cause double execution again. You cannot undo having run the code, and running it twice can have side effects. So now it first tries user dir and later on ignores the same plugin in other directories again.

Overall it just shows again that we need to separate plugin metadata from code for the next plugin system.

Overall I'm quite happy with how well this works now. This really should in practice work as before but using the new APIs.

Also it shows that importlib provides really good interface now for us to implement plugin loading and if done properly we can for the next iteration of the plugin system simplify the loading a lot.

picard/pluginmanager.py

If a plugin is already loaded avoid running its code again. This avoids double execution in case the plugin is in both the user and system plugin directory.

zas

Overall, it looks good, just 2 minor comments

picard/pluginmanager.py

zas

LGTM

hboetes · 2023-09-11T17:24:12Z

Thanks for the speedy fix. One note, there are various plugins that won't work with python-3.12, like the replaygain plugin. Perhaps a heads-up to the plugin developers is warranted.

phw · 2023-09-12T05:28:41Z

@hboetes the replaygain plugin is actually supposed to work after the latest changes from this PR. At least it does so for me. Do you have any different issue now?

If there is compatibility issue with one of the existing plugins we should of course fix it, but I'm currently not aware of one.

phw and others added 3 commits September 9, 2023 12:49

Run CI tests with Python 3.12.0-rc.1

1912420

PICARD-2354: Replaced deprecated imp module in plugin manager

0a56f40

PICARD-2354: Exec plugin module when loading

136e2b7

phw mentioned this pull request Sep 9, 2023

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

Closed

5 tasks

phw added the Bug fix label Sep 9, 2023

phw requested a review from zas September 9, 2023 14:32

github-advanced-security bot found potential problems Sep 9, 2023

View reviewed changes

test/test_plugins.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Sep 9, 2023

View reviewed changes

test/test_plugins.py Fixed Show fixed Hide fixed

phw force-pushed the python-3.12-compat branch from 0ea9a64 to 0e65171 Compare September 9, 2023 14:42

github-advanced-security bot found potential problems Sep 9, 2023

View reviewed changes

test/test_plugins.py Fixed Show fixed Hide fixed

test/test_plugins.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Sep 9, 2023

View reviewed changes

test/test_plugins.py Fixed Show fixed Hide fixed

test/test_plugins.py Fixed Show fixed Hide fixed

phw added 2 commits September 9, 2023 16:55

PICARD-2354: Use MetaPathFinder to find Picard plugins

b1afa8e

PICARD-2355: Use zipimporter.find_spec if available

1944a0e

Avoids use of deprecated and in Python 3.12 removed API

phw force-pushed the python-3.12-compat branch from 0e65171 to 1944a0e Compare September 9, 2023 14:55

pluginmanager: Avoid multiple calls to PathFinder().find_spec()

bcda101

zas requested changes Sep 10, 2023

View reviewed changes

picard/pluginmanager.py Outdated Show resolved Hide resolved

phw added 2 commits September 10, 2023 11:47

Refactor PluginMetaPathFinder

70904c1

PICARD-2751: Avoid plugins with relative imports executing plugin bod…

4db506c

…y twice

phw force-pushed the python-3.12-compat branch from 0e98162 to a7c9504 Compare September 10, 2023 11:11

github-advanced-security bot found potential problems Sep 10, 2023

View reviewed changes

picard/pluginmanager.py Fixed Show fixed Hide fixed

phw force-pushed the python-3.12-compat branch from d9925c0 to 6d9528c Compare September 10, 2023 11:33

phw requested a review from zas September 10, 2023 11:45

phw added 2 commits September 10, 2023 13:47

PICARD-2751: Load plugins in plugin dir priority order

a889752

If a plugin is already loaded avoid running its code again. This avoids double execution in case the plugin is in both the user and system plugin directory.

Moved legacy code out of PluginManager._load_plugin

83ea460

phw force-pushed the python-3.12-compat branch from 6d9528c to 83ea460 Compare September 10, 2023 11:47

zas requested changes Sep 10, 2023

View reviewed changes

picard/pluginmanager.py Outdated Show resolved Hide resolved

picard/pluginmanager.py Outdated Show resolved Hide resolved

PluginManager: Minor code cleanup

7d84330

phw force-pushed the python-3.12-compat branch from 702a769 to 7d84330 Compare September 10, 2023 14:40

zas approved these changes Sep 11, 2023

View reviewed changes

phw merged commit b1c2874 into metabrainz:master Sep 11, 2023
69 checks passed

phw deleted the python-3.12-compat branch September 11, 2023 06:30

phw mentioned this pull request Sep 12, 2023

PICARD-2751: Restore the ability to load compiled .pyc plugins #2309

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

phw commented Sep 9, 2023 •

edited

Loading

hboetes commented Sep 9, 2023

hboetes commented Sep 9, 2023

phw commented Sep 9, 2023

hboetes commented Sep 9, 2023 •

edited

Loading

hboetes commented Sep 9, 2023

zas left a comment

phw commented Sep 10, 2023

zas left a comment

zas left a comment

hboetes commented Sep 11, 2023

phw commented Sep 12, 2023

PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

Conversation

phw commented Sep 9, 2023 • edited Loading

Summary

Problem

Solution

hboetes commented Sep 9, 2023

hboetes commented Sep 9, 2023

phw commented Sep 9, 2023

hboetes commented Sep 9, 2023 • edited Loading

hboetes commented Sep 9, 2023

zas left a comment

Choose a reason for hiding this comment

phw commented Sep 10, 2023

zas left a comment

Choose a reason for hiding this comment

zas left a comment

Choose a reason for hiding this comment

hboetes commented Sep 11, 2023

phw commented Sep 12, 2023

phw commented Sep 9, 2023 •

edited

Loading

hboetes commented Sep 9, 2023 •

edited

Loading