Skip to content
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

Pushing VFS down to converters #123

Conversation

ktsitsi
Copy link
Collaborator

@ktsitsi ktsitsi commented May 30, 2024

This PR:

  • Pushes down the VFS inside the converters allowing passing of config and context for accessing input images directly from S3.

Usage:

from tiledb.bioimg import from_bioimg
from_bioimg(src='s3://tiledb-bioimg/local_testing/CMU-1-Small-Region-rgb.ome.tiff', dest='./tests/data/cmu-rgb-vfs.tdb', reader_kwargs={"config":tiledb.Config({})}
>>Ingesting level 0: 100%|██████████| 9/9 [00:00<00:00, 47.70tiles/s]
>>Ingesting level 1: 100%|██████████| 1/1 [00:00<00:00, 47.95tiles/s]

---
from tiledb.bioimg.openslide import TileDBOpenSlide
slide = TileDBOpenSlide('s3://tiledb-bioimg/local_testing/CMU-1-Small-Region-rgb.ome.tiff', config=tiledb.Config({}))
slide.levels
>>Out[9]: (0, 1, 2, 3)

It will resolve related story [sc-25684] [sc-43577]

@ktsitsi ktsitsi force-pushed the kt/sc-41216/add-support-for-passing-context-to-bioimaging-api-function branch from e1d5862 to b877256 Compare June 14, 2024 08:23
@ktsitsi ktsitsi marked this pull request as ready for review June 14, 2024 12:38
@ktsitsi ktsitsi force-pushed the kt/sc-41216/add-support-for-passing-context-to-bioimaging-api-function branch from 763fbe3 to e8385e2 Compare June 14, 2024 12:42
Copy link

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

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

Looks good to me (left one optional comment, but feel free to implement or ignore).

Comment on lines 22 to 28
self,
input_path: str,
logger: Optional[logging.Logger] = None,
config: Optional[Config] = None,
ctx: Optional[Ctx] = None,
extra_tags: Sequence[Union[str, int]] = (),
):
Copy link

Choose a reason for hiding this comment

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

Suggested change
self,
input_path: str,
logger: Optional[logging.Logger] = None,
config: Optional[Config] = None,
ctx: Optional[Ctx] = None,
extra_tags: Sequence[Union[str, int]] = (),
):
self,
input_path: str,
*,
logger: Optional[logging.Logger] = None,
config: Optional[Config] = None,
ctx: Optional[Ctx] = None,
extra_tags: Sequence[Union[str, int]] = (),
):

It might be worth making optional arguments keyword only so that you don't need to worry about breaking the API when adding in new optional arguments. As implemented, if someone naively tried used the old API with only positional arguments, it would just pass extra_tags to config without a clear error message.

@ktsitsi ktsitsi force-pushed the kt/sc-41216/add-support-for-passing-context-to-bioimaging-api-function branch from 20479a5 to 6f38b8b Compare June 20, 2024 11:02
@ktsitsi ktsitsi merged commit 83a9166 into main Jun 20, 2024
3 checks passed
@ktsitsi ktsitsi deleted the kt/sc-41216/add-support-for-passing-context-to-bioimaging-api-function branch June 20, 2024 11:20
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.

2 participants