Skip to content

Conversation

@DragaDoncila
Copy link
Contributor

@DragaDoncila DragaDoncila commented May 10, 2025

When a plugin is selected by the user to read a file, we want to raise specific error messages when that reader fails to read, for whatever reason.

Prior to this PR:

  • if a selected reader refused to read the file i.e. the get_reader function returned None, the error message would say that the plugin returned no data
  • if a selected reader returned a null_layer_sentinel, no error would be surfaced in napari. I think this is a holdover from olden days when we would cascade readers, so we didn't want the null_layer_sentinel to throw an error. I think there's probably good reason to remove the null_layer_sentinel entirely, but that's a discussion for another day.

With this PR:

  • if a selected reader refused to read the file, we get "Reader {plugin_name!r} was selected to open {paths!r}, but refused the file."
  • if a selected reader tried to read the file and returned null_layer_sentinel, we get "Reader {plugin_name!r} was selected to open {paths!r}, but returned no data."

As an aside, if a plugin is not selected, we get a semi-generic "No readers returned data" but I think this is also ok as, from the napari side, it's ~impossible for npe2 reading to get called without a plugin selected - either we use the preferred one, the user specifically selects one, or we explicitly pass the only one available.

Note that this brings the _is_null_layer_sentinel function to npe2, and it previously lived in napari. I think this is ok, and if we're happy with this PR, we can update its usage in napari to import from here.

Note also that this function checks specifically for a list (same as with napari/#7851), not a container sequence, but I actually think this is also ok and maybe we should update our typing in other places to simply expect a list, since many other container sequences wouldn't be useful anyway: a dict would likely fail somewhere weirdly, a set would lead to unexpected layer orderings... I guess a generator would be ok? But I don't think anyone is returning anything except a list.

@DragaDoncila
Copy link
Contributor Author

ok these tests are unexpectedly failing cause they just passed locally. I think I did something weird. Please hold...

@codecov
Copy link

codecov bot commented May 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (986a170) to head (271e063).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #377   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          37       37           
  Lines        2711     2719    +8     
=======================================
+ Hits         2707     2715    +8     
  Misses          4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DragaDoncila
Copy link
Contributor Author

Ok this remaining error is totally not my fault, I will address in separate PR. Also this will probs break napari tests, so I'll take a look at that too. @napari/core-devs this might be a bigger change than we want to mess with right now, so I'm happy to close the PR if we want to punt and keep the existing behaviour (silent failure on selected reader returning no data), but would appreciate your insights, especially if you play with this stuff often.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 16, 2025

If you saw the original version, I was in the wrong env, naturally.
ignore me.
(btw my issues with mp4 without pyav/ffmpeg are also resolved so that could have been an env issue too 😅 )

True, if the layer_data indicates an empty file, False otherwise
"""
return (
isinstance(layer_data, list)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it always list or can it be like a tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think technically speaking it could be a tuple, but this function was reproduced from napari, so it's what we've always used to validate the return type of readers. I'd say we keep it like this for now, and change it later if need be. Looking at the docs for readers, we always refer to a list also.

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.

3 participants