-
Notifications
You must be signed in to change notification settings - Fork 10
feature/migrate-writers #126
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
e6544a5
feature/v3-ome-zarr-writer
BrianWhitneyAI 975db9e
Merge branch 'main' into feature/v3-ome-zarr-writer
BrianWhitneyAI fd7c568
Merge branch 'main' into feature/v3-ome-zarr-writer
BrianWhitneyAI 1e4420b
update V2 writer to use zarr 3.0.0+
BrianWhitneyAI b6b6d0f
remove generate_zarr
BrianWhitneyAI 19cc5ee
add fsspec[http]
BrianWhitneyAI e95d0f3
feature/migrate-writers
BrianWhitneyAI 1c4a689
update comments
BrianWhitneyAI c4f40b1
update import lib
BrianWhitneyAI cafe0dd
remove unnecessary deps
BrianWhitneyAI 1f69542
add dropped comments
BrianWhitneyAI 12b4c7d
bump tests
BrianWhitneyAI 38c66f8
add fsspec back
BrianWhitneyAI 2dffac0
lock bioio base 1.0.8
BrianWhitneyAI 7cd6615
bump to 1.4.0
BrianWhitneyAI e1fc39b
Writer Documentation
BrianWhitneyAI 248f9e3
Update README.md
BrianWhitneyAI 62b1d6e
comment resolution
BrianWhitneyAI 3f5944c
Merge pull request #129 from bioio-devs/feature/new-writer-docs
BrianWhitneyAI File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| # -*- coding: utf-8 -*- | ||
| #!/usr/bin/env python | ||
|
|
||
| """Top-level package for bioio.""" | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| # -*- coding: utf-8 -*- | ||
| #!/usr/bin/env python | ||
|
|
||
| """Unit test package for bioio.""" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| from typing import Any | ||
|
|
||
| import bioio_base as biob | ||
| from bioio_base.writer import Writer | ||
|
|
||
|
|
||
| class DummyWriter(Writer): | ||
| """ | ||
| A dummy writer for testing bioio.writers entry-point discovery. | ||
| """ | ||
|
|
||
| @staticmethod | ||
| def save( | ||
| data: biob.types.ArrayLike, | ||
| uri: biob.types.PathLike, | ||
| dim_order: str = biob.dimensions.DEFAULT_DIMENSION_ORDER, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """ | ||
| Stub implementation that deliberately isn’t implemented. | ||
| """ | ||
| raise NotImplementedError("DummyWriter.save is a stub") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| from importlib.metadata import entry_points | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| def test_dummy_writer_discovery_and_api(dummy_plugin: str) -> None: | ||
| # Entry-point registration | ||
| eps = entry_points(group="bioio.writers") | ||
| names = {ep.name for ep in eps} | ||
| assert "DummyWriter" in names, f"found: {sorted(names)}" | ||
|
|
||
| # Public API (__all__) | ||
| import bioio.writers as pkg | ||
|
|
||
| assert "DummyWriter" in getattr(pkg, "__all__", []) | ||
|
|
||
| # Import works (dynamic, so ignore mypy attr-defined error) | ||
| from bioio.writers import DummyWriter # type: ignore[attr-defined] | ||
|
|
||
| assert DummyWriter.__name__ == "DummyWriter" | ||
|
|
||
|
|
||
| def test_dummy_writer_save_stub(dummy_plugin: str) -> None: | ||
| # The save() stub should raise NotImplementedError | ||
| from bioio.writers import DummyWriter # type: ignore[attr-defined] | ||
|
|
||
| with pytest.raises(NotImplementedError): | ||
| DummyWriter.save(data=[1], uri="unused", dim_order="XYZ") |
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why log this if
bioio-ome-tiffisn'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that now - weird that we did that. Makes sense to include to until we do a major version change
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
saveshould be removed from BioImage, or (b)savecould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this moving of writers seems to be a breaking change anyway, maybe this is a good opportunity to remove
saveuntil we design something better (abstracted api or not)?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.
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