Add option to use npe2 shim/adaptor for npe1 plugins

[tlambert03]

Description

A "softer/gentler" version of #4015. This PR adds a new setting (settings.plugin.use_npe2_adaptor) that lets the user opt-in to the npe2-adaptor behavior. It defaults to False, but when set to True, it will essentially treat all plugins as npe2 plugins. This means that:

• upon first import after enabling this setting, npe1 plugins will imported as usual, but their contributions will be indexed and stored in a cached npe2 "shim" manifest (see NPE1Adapter Part 3 - caching of adapter manifests npe2#126).
• after the first time running napari with this option on, later boots of napari will incur no penalty for importing and discovering plugin contributions. 🎉
• each plugin will be imported lazily, only when one of their contributions is requested.
• (adapter caches can be managed using npe2 cache --help)
• the discovery method on the old napari.plugins.plugin_manager becomes a no-op. In a later version, i hope to make this shim the only option for npe1, at which point all the npe1-specific code in the repo can go away (like [WIP] Use npe2 shim/adaptor for npe1 plugins. remove npe1 plugin code! #4015)

Type of change

• Bug-fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

References

How has this been tested?

• example: the test suite for my feature covers cases x, y, and z
• example: all tests pass with my change

Final checklist:

• My PR is the minimum possible work for the desired functionality
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• If I included new strings, I have used trans. to make them localizable.
  For more information see our translations guide.

[codecov]

Codecov Report

Merging #4564 (0e99a2c) into main (640484c) will increase coverage by 1.57%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #4564      +/-   ##
==========================================
+ Coverage   86.07%   87.64%   +1.57%     
==========================================
  Files         587      587              
  Lines       50384    50412      +28     
==========================================
+ Hits        43366    44184     +818     
+ Misses       7018     6228     -790     

Impacted Files	Coverage Δ
napari/_qt/dialogs/qt_plugin_dialog.py	62.36% <40.00%> (-0.08%)	⬇️
napari/plugins/__init__.py	82.35% <75.00%> (-2.03%)	⬇️
napari/_qt/menus/plugins_menu.py	92.95% <100.00%> (+0.10%)	⬆️
napari/plugins/_npe2.py	88.77% <100.00%> (+0.23%)	⬆️
napari/settings/_base.py	92.01% <100.00%> (+0.54%)	⬆️
napari/settings/_plugins.py	96.29% <100.00%> (+0.14%)	⬆️
napari/plugins/pypi.py	45.61% <0.00%> (-0.82%)	⬇️
napari/utils/config.py	100.00% <0.00%> (ø)
napari/layers/utils/string_encoding.py	100.00% <0.00%> (ø)
napari/components/experimental/monitor/_api.py	0.00% <0.00%> (ø)
... and 55 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 640484c...0e99a2c. Read the comment docs.

[DragaDoncila]

I really like this. Pulled it down and it works like a charm. Only "issue" I've discovered is that plugins are now indexed strictly by package name, whereas for npe1 plugins I think it was module? E.g. a "shimmed" napari_tifffile now shows up as napari-tifffile. Not a huge deal, but can break preferences.

[DragaDoncila]

what is type(manifest).__name__.lower() actually checking sorry? Just curious how we're discerning between the different manifests, I have no issue with it.

[tlambert03]

it's checking the name of the class, to make sure it's NPE1Adapter 😂 ... it could be an isinstance, but requires an import from private module. I dunno 🤷

[DragaDoncila]

I think that's fine, but I think it would be more explicit to just have 'shim' if type(manifest).__name__ == 'NPE1Adapter' else 2 and then even a comment that we're avoiding a private import

[DragaDoncila]

Ok a bigger issue is that now builtins doesn't work, and napari (the npe2 plugin) only declares ['*.csv', '*.npy'] in its filename_patterns.

Also, oddly, if I call

viewer.open('../data/man_seg002.tif', plugin='napari')

I get

ValueError: There is no registered plugin named 'napari'.
Names of plugins offering readers are: set()

but if I have napari-tifffile installed and run viewer.open('../data/man_seg002.tif', plugin='napari'), it opens with napari-tifffile?

viewer.open('../data/man_seg002.tif', plugin='napari')
Out[3]: [<Image layer 'man_seg002 [1]' at 0x7fa5146ba260>]

viewer.layers[-1].source
Out[4]: Source(path='../data/man_seg002.tif', reader_plugin='napari-tifffile', sample=None, widget=None)

FWIW I don't think this is directly tied to this PR, but maybe just leftover because we still have both builtins and napari lying around? I'm just wondering whether, now that we're shimming, this will become even muddier.

See also #4575 where @jni and I realized we probably need to show both.

[tlambert03]

with #4668 merged into this PR, these issues seem fixed. @DragaDoncila can you confirm?

[DragaDoncila]

@tlambert03 yep, definitely. The issue was more with our manifest than with the shim. I didn't realize initially that in builtins.yaml we were pointing to the same reader function as we always have, and just hadn't correctly added all the filename patterns.

[DragaDoncila]

Although I had already opened a PR for this... But we just missed it in release. @tlambert03 see #4580

[tlambert03]

with #4668 merged into main, I think this is working. Would be good to get some eyes on this from @napari/core-devs

[DragaDoncila]

I love this, works as expected and is super exciting 🎉 . I have just a few comments, which I don't think need to be addressed now, while this is still optional, but should be addressed before we remove the original napari_plugin_manager entirely:

• When setting the preference for the first time, or installing new plugins (in the GUI), a restart is required for the plugins to be shimmed. We may want to add a warning or some such, and ideally for newly installed plugins, they would be shimmed on install so the user doesn't have to restart.
• I played around with a few plugins and most converted just fine. However, when I installed btrack and then restarted napari, I got napari.manifest -> 'btrack' could not be imported: Could not find file 'napari.yaml' in module 'btrack' in the terminal. I assume there was some conversion issue, but it would be nice if this was flagged more prominently for the GUI user somehow...
• I think some plugins will now change their registered name in the plugin manager, and this may lead to preferences being incorrect. In the GUI I think this is okay as the user will be prompted to select a different plugin (so they have an out), but maybe we could let them know explicitly why this is happening, or convert their preferences silently maybe? Not sure about that last bit.

[DragaDoncila]

I think that's fine, but I think it would be more explicit to just have 'shim' if type(manifest).__name__ == 'NPE1Adapter' else 2 and then even a comment that we're avoiding a private import

[DragaDoncila]

So just to clarify, if I use this option I will never see an npe1 plugin (what a dream...). For my own reference, do we have an idea of what percentage of current npe1 plugins (if any) cannot be auto converted?

[tlambert03]

For my own reference, do we have an idea of what percentage of current npe1 plugins (if any) cannot be auto converted?

when I created the shim, I tested all plugins (here: napari/npe2#132) ... at the time 21 out of the 143 plugins tested actually need the shim thing at all (in the sense that they had locally defined contributions). all the rest have direct python_names for all their assets. And the shim strategy appeared to work for all 21 of those. So, based on those tests, this should work fine for all plugins. The one exception is napari tools menu, which is not actually a plugin, but a monkeypatch that just hijacks the loading system

[tlambert03]

I got napari.manifest -> 'btrack' could not be imported: Could not find file 'napari.yaml' in module 'btrack'

[tlambert03]

I got napari.manifest -> 'btrack' could not be imported: Could not find file 'napari.yaml' in module 'btrack'

thanks will have a look

[tlambert03]

I played around with a few plugins and most converted just fine. However, when I installed btrack and then restarted napari, I got napari.manifest -> 'btrack' could not be imported: Could not find file 'napari.yaml' in module 'btrack' in the terminal. I assume there was some conversion issue, but it would be nice if this was flagged more prominently for the GUI user somehow...

looked into this. It's unrelated to this PR and is a packaging issue in btrack. Fixed submitted quantumjot/btrack#153

[jni]

I really like this too. As noted in private conversation with @tlambert03, it would be good if the option linked to text that explained the consequences of that choice, which are:

• shimmed plugin widgets lose the ability to specify in which docked area they live (left, right, bottom)
• reader call order isn't a thing anymore

Both of those are going away though so I'm happy to ease users into the transition with this. Good stuff.

[nclack]

I love this so much.

Agree with @jni, it would be good to note the behavioral differences for the user. The startup behavior is another thing that should be added to that list - especially since it should be a positive for most folks.

[andy-sweet]

I don't understand all the implications of this PR, but the changes looks good. Maybe a missing translation.

[tlambert03]

I added a document with some additional explanation at https://github.com/napari/napari/pull/4564/files#diff-37b9bdb893f94908825ca80e038f5912fcf30bbc91f34eb313529680c16a14d1

I'm not sure how to link to it in the preferences pane (I tried an anchor tag but it doesn't work). The tooltip does pop up, but only if you hover over the checkbox (not the label). I think it's a bit out of scope here to figure out how to add web links to the preferences pane. so hopefully having the document there is enough

[jni]

❤️

[jni]

Thanks everyone for the reviews! I'll give this another ~24h before merging (or @tlambert03 feel free to self-merge if it's just sitting here without comment).

The macos 3.10 build is failing which what appears to be a numcodecs compilation error? That could be due to an updated macos image. I notice that there's no 3.10 wheels for numcodecs, but a new release is being worked on (0.10.0a3 was released in March, and does have 3.10 wheels), so presumably this problem will resolve itself. If we want to fix it beforehand, installing numcodecs with conda is an option. But either way, the failure is unrelated to this PR.