-
-
Notifications
You must be signed in to change notification settings - Fork 443
Update builtins read extensions #7826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Do we have a centralized place to find all these additions? Or are you just remembering them? I should rpobably add them in the code snippet so we can remake this next time easily. |
new code import imageio
import tifffile
from pprint import pp
imageio_exts = {f'*{ext}' for format in imageio.formats for ext in format.extensions}
tifffile_exts = {f'*.{ext}' for ext in tifffile.TIFF.FILE_EXTENSIONS}
napari_exts = {f'*.{ext}' for ext in ('npy', 'zarr', 'csv')}
exts = imageio_exts | tifffile_exts | napari_exts
pp(sorted(list(exts)), compact=True) ['*.3fr', '*.arw', '*.avs', '*.bay', '*.bif', '*.bmp', '*.bmq', '*.bsdf',
'*.btf', '*.bufr', '*.bw', '*.cap', '*.cine', '*.cr2', '*.crw', '*.cs1',
'*.csv', '*.ct', '*.cur', '*.cut', '*.dc2', '*.dcm', '*.dcr', '*.dcx', '*.dds',
'*.dicom', '*.dng', '*.drf', '*.dsc', '*.ecw', '*.eer', '*.emf', '*.eps',
'*.erf', '*.exr', '*.fff', '*.fit', '*.fits', '*.flc', '*.fli', '*.fpx',
'*.ftc', '*.fts', '*.ftu', '*.fz', '*.g3', '*.gbr', '*.gdcm', '*.gel', '*.gif',
'*.gipl', '*.grib', '*.h5', '*.hdf', '*.hdf5', '*.hdp', '*.hdr', '*.ia',
'*.icns', '*.ico', '*.iff', '*.iim', '*.iiq', '*.im', '*.img', '*.img.gz',
'*.ipl', '*.j2c', '*.j2k', '*.jfif', '*.jif', '*.jng', '*.jp2', '*.jpc',
'*.jpe', '*.jpeg', '*.jpf', '*.jpg', '*.jpx', '*.jxr', '*.k25', '*.kc2',
'*.kdc', '*.koa', '*.lbm', '*.lfp', '*.lfr', '*.lsm', '*.mdc', '*.mef',
'*.mgh', '*.mha', '*.mhd', '*.mic', '*.mnc', '*.mnc2', '*.mos', '*.mpo',
'*.mri', '*.mrw', '*.msp', '*.ndpi', '*.nef', '*.nhdr', '*.nia', '*.nii',
'*.nii.gz', '*.npy', '*.npz', '*.nrrd', '*.nrw', '*.ome.tif', '*.orf', '*.pbm',
'*.pcd', '*.pcoraw', '*.pct', '*.pcx', '*.pef', '*.pfm', '*.pgm', '*.pic',
'*.pict', '*.png', '*.ppm', '*.ps', '*.psd', '*.ptif', '*.ptiff', '*.ptx',
'*.pxn', '*.pxr', '*.qpi', '*.qptiff', '*.qtk', '*.raf', '*.ras', '*.raw',
'*.rdc', '*.rgb', '*.rgba', '*.rw2', '*.rwl', '*.rwz', '*.scn', '*.seq',
'*.sgi', '*.sr2', '*.srf', '*.s
8000
rw', '*.sti', '*.stk', '*.svs', '*.swf',
'*.targa', '*.tf2', '*.tf8', '*.tga', '*.tif', '*.tiff', '*.vtk', '*.wap',
'*.wbm', '*.wbmp', '*.wdp', '*.webp', '*.wmf', '*.xbm', '*.xpm', '*.zarr',
'*.zif'] |
I was tipped off by the failing tests and then looked at the diff. Looking at the code: napari/napari_builtins/io/_read.py Line 161 in 95ced67
splits out zarr napari/napari_builtins/io/_read.py Line 75 in 95ced67
considers npy napari/napari_builtins/io/_read.py Line 380 in 95ced67
reads csv I don't see any other special cases? Oh, wait: napari/napari_builtins/io/_read.py Line 93 in 95ced67
.lsm being shifted to tifffile. I think we should put all the tifffile ones here? Then again I think imageio uses tifffile, so that whole if might not be needed. |
Thanks, yeah makes sense!
mmm, but here we are in code territory, no longer yaml manifest, so we can actually do this programmatically on the fly if needed. Either way, I'd say that's a separate PR! |
Yeah I think this code was all written a long time ago -- or ventured from scikit-image a long time ago -- so it may not use best practices for imageio v3. Probably worth making a followup issue. One other thing -- this touches on our reader arch in general, but i just realized it (again):
Which is nigh useless. Meanwhile, imageio gives a nice error:
But it's swallowed by our reader. Anyhow, with this change, which is strictly correct because we don't depend on the video plugins to imageio, now those files just won't work and there is no easy way for someone to get them to work. I quickly searched the napari-hub and didn't find anything obvious. |
Also, i tested this with a .ndpi, ome.tiff, .qptiff, and .svs (sample images used QuPath, but also common formats used at JAX because there are used by slide scanners) and none of them worked -- they all need I think adding the extensions is still a plus, because again otherwise there isn't any easy way for someone to get these files to work without changing the extensions, but I think we need to try to improve error handling and ensure the user knows what to do when they try to load an .svs or ndpi. |
Ouch, yeah, looks like we need to surface those more informative errors all the way to the user. I guess the only real solution (other than moving away from explicit extensions) would be to generate this yaml periodically with a workflow which gets the extensions from the various libraries? |
Just to chime in here on that annoying error message (which I also hate lol). We added that when updating the reading behaviour to stop cascading the different reader plugins and move to the reader dialog. The tricky bit it was supposed to address is when: the user has a plugin associated with the extension and that plugin fails and/or that plugin is the only one that declares it can read the file. Then the user was supposed to get this error message that explains why the plugin was tried in the first place. It's annoying how confused the stack trace is though. When we raise the error we raise it |
I don't think our cursed reader is a good reason to move away from explicit extensions at all lol. The vast majority of plugins will only declare a handful of extensions and for those that don't, it's up to them to make sure that list is up to date 🤷♀️. I think a workflow that regenerates the list periodically is a good idea. |
@DragaDoncila How do you think we should handle, for example, the video extensions or the tiff variants then? |
Yeah this is what I think we should do. The original |
So, for this PR, I guess we need to add these extensions (@psobolewskiPhD do you know where I can find them programmatically, like I did with the rest?) And then we follow up with:
|
@brisvag Edit: ok i found it, it's not well documented in the docs, but I found a sphinx extension That one shows: So we can use:
You don't need to have the extensions installed for this to work.
And it appears to match what we currently include, which would explain where they came from. |
Awesome, thanks for tracking that down @psobolewskiPhD! I updated everything, should be good to go now. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7826 +/- ##
==========================================
+ Coverage 92.92% 92.94% +0.02%
==========================================
Files 641 641
Lines 60627 60600 -27
==========================================
- Hits 56335 56323 -12
+ Misses 4292 4277 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Looks good to me. I tested with an svs and mp4 I had handy and both failed, but worked once i installed av and imagecodecs. |
I created a followup issue regarding the error messages: Also, Im bumping this issue: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have spoken too soon.
video files do work, but I'm not sure that tiff variants, like the issue are actually solved.
I have more test files on my work laptop, I will try to test again tomorrow.
I moved the milestone while you are testing. If this becomes mergeable before the release we can move it back. |
OK, I think the reason I can't open the svs I have on my personal mac -- meaning this doesn't fix the issue-- is because of this check: napari/napari_builtins/io/_read.py Line 532 in 1ab3ac3
The tifffile extensions are not in imageio.formats, so they arn't in READER_EXTENSIONS. We need So we can merge this as is, but we should ensure to not close the issue.
Or another option is to use: |
I like this option the most, assuming we have the better error surfaced to the user. I would vote for merging this (as it's strictly better than main, I think?), but follow up with the above PRs for error messages and to use |
Eh, is it better? It's in the eye of the beholder I think: I do agree that we need 3 fixes:
I think they are all interdependent, but the 2nd is pretty much its own independent thing, it affects all readers. Meanwhile, the first will be a silent change without the 3rd. |
I'd like to try and fix the error as suggested, but this part of the codebase is a bit out of my comfort zone... @DragaDoncila you think you could point me in the right direction? |
@brisvag sorry I missed this! I'll draft up a PR tomorrow and we can review together when we pair, that way you also get more familiarity with that part of code |
After pairing with @DragaDoncila, we concluded that the Additionally, we are able to modify the check for |
@brisvag Oh that makes sense. Just handle the extensions on the yaml side! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve, but think it's worth considering just going full iio.imread.
I made a PR to this branch.
Great simplification @psobolewskiPhD, merged it! Now we just give everything to imread except for |
In theory we could split off npy at the very beginning like csv? But I guess |
I can see it either way. I'd say for now let's leave it as is since it's not really changing behaviour. |
) # References and relevant issues Discussed in #7826 # Description When a plugin is selected to read a file, but it fails OR when there is only a single plugin available to read a file, we raise our own error message `from` the original error. However, this means the (likely more useful) original error thrown by the plugin doesn't appear unless you scroll up in the stack trace, even if the error would be relatively easy to fix e.g. just install `imagecodecs` or something. This PR improves our own error message so that the original error is printed to the user, and they are advised to scroll up to see the full stack trace. @brisvag and I discussed maybe bringing the actual stack trace down, but decided that would be a lot more complex, and potentially confusing for developers who **are** used to just scrolling up to get to their own code. Prior to this PR, if you don't have `tifffile` installed (for some reason) and try to open a tiff file:  With this PR:  --------- Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com> Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com> Co-authored-by: Draga Doncila <ddon0001@student.monash.edu>
References and relevant issues
Closes #3828.
Description
List of new extensions generated with:
This spits out json formatted and ready to be copy pasted in the file: