-
Notifications
You must be signed in to change notification settings - Fork 50
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
Use the Zarr's new getitems()
API
#131
Conversation
aca8ed9
to
dd82a5e
Compare
…o zarr_getitems
…o zarr_getitems
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.
Thanks @madsbk, I think we can do a little bit of cleanup, to avoid repeating ourselves with the minimum supported version.
python/kvikio/zarr.py
Outdated
finally: | ||
for f in files: | ||
f.close() |
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.
The idiomatic way to do this is...
import contextlib
...
with contextlib.ExitStack() as stack:
for key in keys:
f = stack.enter_context(kvikio.CuFile(filepath, "r"))
ret[key] = ..
io_results.append((f.pread(ret[key]), nbytes)
for fut, nbytes in io_results:
nbytes_read = fut.get()
...
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'll note that both the new code (and my proposed approach) leave dangling futures if the block produces an exception anywhere. I guess that is fine because that propagates and the program is hosed at that point anyway?
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 think that is ok
…o zarr_getitems
Thanks @wence-, I think I have addressed all of your issues. |
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.
Thanks Mads.
/merge |
I think we need to wait for the other codeowners to approve too. |
Thanks all! 🙏 Great work 🥳 |
By using the new API in zarr-developers/zarr-python#1131, we do not have to guess whether to read into host or device memory. That is, no more filtering of specify keys like:
Notice, this PR is on hold until Zarr v2.15 is released
Closes #119
UPDATE: Zarr v2.15 has been released