-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding PNG support and enabling experimental reader in kwargs #654
Conversation
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.
@ktsitsi "experimental_reader" sounds like a feature that might go away in the future or become the new default, at which we'd want to remove the option.
Is there a way that we can support more than one reader without using a flag that will eventually become deprecated? What about reader=experimental
and reader=production
(the default)?
Indeed it is the latter the experimental reader will be the new default. I will change it as you recommend and I will assign the selection inside the function so we avoid the depreciation. |
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.
Sounds good @ktsitsi !
106918e
to
b731170
Compare
src/tiledb/cloud/bioimg/ingestion.py
Outdated
|
||
# Get either the new experimental or default reader | ||
reader = kwargs.pop("reader", "production") | ||
experimental_reader = True if reader == "experimental" else False |
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.
Should this be an enum?
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.
You are right @ihnorton I changed it in my last commit. I just thought that since the old reader is about to be deprecated once the experimental becomes the default one, that I did not want to over-engineer it. But it is cleaner this way thanks!
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.
@ktsitsi I made a suggestion/question inline.
@@ -358,8 +364,8 @@ def register_dataset_udf( | |||
compressor = kwargs.pop("compressor", None) | |||
|
|||
# Get either the new experimental or default reader | |||
reader = kwargs.pop("reader", "production") | |||
experimental_reader = True if reader == "experimental" else False | |||
reader = kwargs.pop("reader", ReaderType.PRODUCTION.value) |
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.
@ktsitsi how about making reader
a named keyword argument of ingest()
instead?
I'm doing a deep dive into our ingestion code right now and the way we have been moving things in and out of kwargs
obfuscates the code.
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.
It indeed does however, the reason I did not do it in the first place is to avoid deprecating it later and breaking the API in the next releases.
44c7079
to
ebece9a
Compare
This PR:
experimental_reader
in the Cloud-py ingestor which was introduced in tiledb-bioimg 0.3.0 and onwards