Add specific error when reader plugin was chosen but failed

[DragaDoncila]

Currently we raise the same error in read when no layer data is returned by any of the tried readers, regardless of whether a plugin name was given or not. This PR updates the error message to be specific when a plugin_name is passed.

This has the added benefit of fixing this issue over in napari-ome-zarr (which is actually a reincarnation of what napari #4026 was trying to solve) by ensuring the error is re-raised over on the napari side.

I thought about whether a fix should go in napari instead, but I think this is the right place to fix this. Over in napari we would have to track somehow whether a plugin was tried or not so that we could know whether to continue with npe1 plugins. But I think if the user chose a plugin, and it failed, npe2 should alert to that, rather than shuffling it under a general "there were no readers that could read this" error.

Before this PR, running e.g. napari --plugin napari-ome-zarr ./non-zarr-folder/ gave:

Traceback (most recent call last):
  File "/opt/miniconda3/envs/omezarr/bin/napari", line 8, in <module>
    sys.exit(main())
  File "/Users/ddoncilapop/CZI/napari/napari/__main__.py", line 570, in main
    _run()
  File "/Users/ddoncilapop/CZI/napari/napari/__main__.py", line 344, in _run
    viewer._window._qt_viewer._qt_open(
  File "/Users/ddoncilapop/CZI/napari/napari/_qt/qt_viewer.py", line 867, in _qt_open
    self.viewer.open(
  File "/Users/ddoncilapop/CZI/napari/napari/components/viewer_model.py", line 1076, in open
    self._add_layers_with_plugins(
  File "/Users/ddoncilapop/CZI/napari/napari/components/viewer_model.py", line 1276, in _add_layers_with_plugins
    layer_data, hookimpl = read_data_with_plugins(
  File "/Users/ddoncilapop/CZI/napari/napari/plugins/io.py", line 104, in read_data_with_plugins
    raise ValueError(
ValueError: There is no registered plugin named 'napari-ome-zarr'.

Whereas now it gives:

Traceback (most recent call last):
  File "/opt/miniconda3/envs/omezarr/bin/napari", line 8, in <module>
    sys.exit(main())
  File "/Users/ddoncilapop/CZI/napari/napari/__main__.py", line 570, in main
    _run()
  File "/Users/ddoncilapop/CZI/napari/napari/__main__.py", line 344, in _run
    viewer._window._qt_viewer._qt_open(
  File "/Users/ddoncilapop/CZI/napari/napari/_qt/qt_viewer.py", line 867, in _qt_open
    self.viewer.open(
  File "/Users/ddoncilapop/CZI/napari/napari/components/viewer_model.py", line 1076, in open
    self._add_layers_with_plugins(
  File "/Users/ddoncilapop/CZI/napari/napari/components/viewer_model.py", line 1276, in _add_layers_with_plugins
    layer_data, hookimpl = read_data_with_plugins(
  File "/Users/ddoncilapop/CZI/napari/napari/plugins/io.py", line 77, in read_data_with_plugins
    res = _npe2.read(paths, plugin, stack=stack)
  File "/Users/ddoncilapop/CZI/napari/napari/plugins/_npe2.py", line 61, in read
    raise e from e
  File "/Users/ddoncilapop/CZI/napari/napari/plugins/_npe2.py", line 55, in read
    layer_data, reader = io_utils.read_get_reader(
  File "/Users/ddoncilapop/CZI/npe2/npe2/io_utils.py", line 66, in read_get_reader
    return _read(
  File "/Users/ddoncilapop/CZI/npe2/npe2/io_utils.py", line 167, in _read
    raise ValueError(f"Plugin {plugin_name} was selected to open "+
ValueError: Plugin napari-ome-zarr was selected to open ['./non-zarr-folder/'], but returned no data.

[codecov]

Codecov Report

Merging #276 (30be9a0) into main (2bf8655) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #276   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines         2813      2815    +2     
=========================================
+ Hits          2813      2815    +2     

Impacted Files	Coverage Δ
npe2/io_utils.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[DragaDoncila]

if we agree this should be the fix I'll fix the napari test also

[tlambert03]

ValueError: There is no registered plugin named 'napari-ome-zarr'. -> ValueError: Plugin napari-ome-zarr was selected to open ['./non-zarr-folder/'], but returned no data.

this is of course a million times better... and I have no issues with this PR, go for it.

... but I don't see why this PR would have that effect on the napari side? From what I can tell, it simply changes the message one would receive, but still raises a ValueError? whereas in your example the full stack trace changed (and read_data_with_plugins went from line 104 to line 77). Are you sure this PR was the only change? I would have definitely assumed that the bad "no plugin registered" problem needed to be fixed on the napari side (and which would be fixed most easily by getting rid of all the old npe1 cruft and just using this npe2 logic)

edit oops i see that it was a different place in read_data_with_plugins that raised the exception... not a code change. but i still don't see why until i look at the code closer... wouldn't it be a ValueError from npe2 either way?

[DragaDoncila]

oops i see that it was a different place in read_data_with_plugins that raised the exception... not a code change. but i still don't see why until i look at the code closer... wouldn't it be a ValueError from npe2 either way?

@tlambert03 yeah it's kinda insidious but this line catches the No readers returned data error from npe2 and then in napari this line assumes that if we have no data, a plugin hasn't been tried at all, so we should carry on with npe1. I'm basically satisfying that assumption by raising a different error when a specific plugin was tried and didn't return data - that ensures we raise an error before we proceed with the npe1 code which eventually led to There is no registered plugin.

[DragaDoncila]

Also @tlambert03 how do I go about making the corresponding PR to fix napari tests? Do we leave this open and then I make the PR there? Or do we merge & release this and then make a PR bumping npe2 version and fixing the test?

[tlambert03]

but this line catches the No readers returned data error from npe2 and then in napari this line assumes that if we have no data, a plugin hasn't been tried at all, so we should carry on with npe1

oh ffs 😂 I had a feeling it was going to be something like that... and I even have a vague feeling of having to use that hack for some reason here. sure, by all means, merge this and then kill that code in napari ASAP

[tlambert03]

Do we leave this open and then I make the PR there? Or do we merge & release this and then make a PR bumping npe2 version and fixing the test?

your call. since (as far as I can tell) the only thing failing is that napari is asserting a specific error string match, you might just soften that test on the napari side and simply assert a ValueError... but the order of merge operations doesn't seem super critical to me. (fine to merge this with failing napari tests for a few days for example)

[DragaDoncila]

oh ffs 😂 I had a feeling it was going to be something like that... and I even have a vague feeling of having to use that hack for some reason here. sure, by all means, merge this and then kill that code in napari ASAP

Yeah, this seemed like the smallest change that would do what we want, and it has the added effect of being a nicer error message here too. But agree, npe1 code needs to go

[DragaDoncila]

I'm going to merge this. I think once we get #275 merged maybe we can do a release?