Don't attempt to use npe1 readers in napari.plugins._npe2.read

[p-j-smith]

Fixes/Closes

Closes #5738

Description

npe2 0.7.0 raises two different error messages if a reader fails to return data.

This pr ensures both messages are caught and swallowed to prevent the ValueError from being raised in the gui.

References

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

How has this been tested?

• added tests that cover changes
• 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 #5739 (ccfcc88) into main (49fee1d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5739      +/-   ##
==========================================
+ Coverage   89.82%   89.85%   +0.03%     
==========================================
  Files         610      610              
  Lines       52131    52142      +11     
==========================================
+ Hits        46825    46851      +26     
+ Misses       5306     5291      -15     

Impacted Files	Coverage Δ
napari/plugins/_npe2.py	91.83% <100.00%> (+0.11%)	⬆️
napari/plugins/_tests/test_npe2.py	100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

[brisvag]

I know there's been past discussion on this, but I haven't been following, so sorry if this has been discussed already: why do we want to suppress those errors? They seem useful info to me.

[DragaDoncila]

@brisvag I commented in #5738 but I'll mention here too. In general we don't want to catch reading errors, and we've made changes on the npe2 side and in napari to make sure reader errors get surfaced sooner.

The original error we were catching here - the No readers returned data error is actually an indicator that no plugin was tried at all. We catch this so that we can continue looking for the correct plugin among npe1 plugins. We'd remove this behaviour alongside all other npe1 code.

As I mentioned on the issue, I played around with the opening behaviour this morning and it all behaved as expected so I'm waiting to hear back about the particular use case we've broken, and then we'll know more about what the best fix is.

[DragaDoncila]

@p-j-smith I've left comments with what I think the approach should be. Let me know if this addresses the original behavior you were seeing! This is unfortunately somewhat of a bandaid fix, but the current state essentially breaks all npe1 readers, I think it's a better approach than the bigger refactor we really need.

[DragaDoncila]

As I mentioned, I think we want to make sure we do raise the Plugin was selected to open error here - it's meant to indicate that the user selected a specific plugin, this plugin was tried, and it failed. Definitely we want to alert the user to this.

What I was missing before is that (because of the npe1 compatibility code) this function may be passed a non-npe2 plugin, and in that case, we don't want to raise an error because we still have npe1 plugins to try.

As such, I think we should instead add the following check to the top of this function (maybe below the stack assertion):

# plugin was passed but is not an npe2 reader
if plugin and plugin not in get_readers():
    return None

I would also add a comment above the "No readers returned data" check:

# plugin wasn't passed and no reader was found
if 'No readers returned data' not in str(e):
    raise

[p-j-smith]

Thanks for the explanation - that makes a lot more sense that what I thought was happening. I assumed that the suppressed errors would somehow be raised here

I've added your suggestions and it works nicely!

[DragaDoncila]

I assumed that the suppressed errors would somehow be raised here

Ahh yes, that's a GUI specific bit of code to allow users to select a different plugin if their saved plugin for a given path failed. The tests above occur without the GUI (or even a napari viewer), so that piece of code doesn't get a chance to handle anything. Even in the GUI scenario that error would've been swallowed, and I think we'd instead get whatever error (if any) was thrown at the end of trying npe1 plugins.

[p-j-smith]

ah I see, thanks for the further info :)

[DragaDoncila]

Here we expect the ValueError to be raised, because the user chose an npe2 plugin but it didn't work. We should also add an additional test for a non npe2 plugin so:

    mock_pm.commands.get.reset_mock()
    assert (
        _npe2.read(["some.randomext"], stack=True, plugin='not-npe2-plugin') is None
    )
    mock_pm.commands.get.assert_not_called()

    with pytest.raises(ValueError, match=f"Plugin '{PLUGIN_NAME}' was selected"):
        _npe2.read(["some.randomext"], stack=True, plugin=PLUGIN_NAME)

[p-j-smith]

I've added these tests and they're passing, thanks :)

[p-j-smith]

as napari.plugins._npe2.read now returns if the plugin name is for an npe1 reader, should the npe1_path variable in this function be renamed?

[DragaDoncila]

should the npe1_path variable in this function be renamed?

I think it's not a very descriptive name to begin with, having tracked down the original PR where it was added, but I don't think it's a worse name now than it was before - in the interest of minimal changes, I would just leave it. I expect this will just be one of the many places we'll need to refactor when we get rid of the npe1 compatibility layer.

[DragaDoncila]

oooops, sorry @p-j-smith min_req tests are failing now - we should split out and skip this test if npe2 < 0.7:

@pytest.mark.skipif(npe2.__version__ < '0.7.0', reason='Older versions of npe2 do not throw specific error.')
def test_read_with_plugin_failure(mock_pm: 'TestPluginManager'):
    with pytest.raises(
        ValueError, match=f"Plugin '{PLUGIN_NAME}' was selected"
    ):
        _npe2.read(["some.randomext"], stack=True, plugin=PLUGIN_NAME)

[p-j-smith]

ooo thanks for spotting that and providing the new test

[DragaDoncila]

I've just pulled this down locally and made sure it works, so I think we're good to go! Test failure looks unrelated but will confirm before merge.

[DragaDoncila]

Confirmed test failure is unrelated. I'm going to merge this now as I think it's important we get it in - otherwise non-shimmed npe1 readers are broken on main with latest npe2. Thanks for the contribution @p-j-smith 🎉 !

[p-j-smith]

thanks for merging @DragaDoncila, and thanks for the review!