feature/migrate-writers #126
Conversation
BrianWhitneyAI
requested review from
SeanDuHare,
aswallace,
evamaxfield,
hughes036,
kmitcham,
pgarrison and
toloudis
|
Documentation is a question here. Where to have writer documentation... |
|
|
||
| OmeTiffWriter = getattr(_writers, "OmeTiffWriter", None) | ||
|
|
||
| if OmeTiffWriter is None: |
There was a problem hiding this comment.
Why log this if bioio-ome-tiff isn't installed? Wouldn't we also need to do this for ome-zarr if we go this direction?
There was a problem hiding this comment.
We implement a specific save function built into BioImage that defaults to the OmeTiffWriter. It never had the option of the OmeZarrWriter. This just maintains the old functionality but warns the user if OmeTiffWriter isnt installed since it comes from bioio-ome-tiff now.
There was a problem hiding this comment.
Oh I see that now - weird that we did that. Makes sense to include to until we do a major version change
There was a problem hiding this comment.
yeah this is another argument for a more involved ABC in bioio_base for writers. Then we could implement a way to choose what writer to use for save @toloudis what do you think about this?
There was a problem hiding this comment.
This was written when OME-TIFF was clearly the only important open file format we wanted, and before we were dealing with multi-TB multiscene time series, nevermind chunked zarr 😄 . Right now I question whether a single save function in the BioImage class even is useful.
The code change as implemented here seems to make sense for maintaining existing functionality...
but I think long term if writers are being removed from bioio, then either (a) save should be removed from BioImage, or (b) save could get a abstract Writer passed in and the caller would have to do the importing and constructing the writer object.
There was a problem hiding this comment.
Since this moving of writers seems to be a breaking change anyway, maybe this is a good opportunity to remove save until we design something better (abstracted api or not)?
There was a problem hiding this comment.
If we have plans to add it back later I think that the current option is a better one for now. Itd be strange to have one release without the save function. hopefully this will be minimally breaking since all the imports remain the same and the usage is the same
| __all__: List[str] = [] | ||
|
|
||
| # Discover all registered writers | ||
| _eps = _entry_points(group="bioio.writers") |
There was a problem hiding this comment.
how do writers get registered? This seems to need some documentation somewhere maybe?
There was a problem hiding this comment.
you add an entry point called bioio.writers to the pyproject.toml (of the plugin). Definitely agree on documentation.
There was a problem hiding this comment.
I'll open another documentation branch once the writers are on main for their respective plugins. That way we can reference directly.
Co-authored-by: Sean LeRoy <[email protected]>
Writer Documentation
4 participants
Description
This PR migrates the writers to their respective reader while maintaining the import structure. It resolves #123 #124 and #125 when combined with its companion PRS. If you have the plugin installed, for example
bioio-ome-tiffthen users can access via the following import:This logic is tested with the DummyPlugin to prove modularity.
Since we offer
BioImage.savethis PR conditionally imports it if the plugin is installed. Otherwise warns the user that it needs to be installed.The goal here is to have this be as seamless to users. Anyone who implements one of the readers should only have to add the necessary plugin to their dependencies without making any code changes.