-
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
feat: add HTTP store support #61
Conversation
No relevant API changes since 0.18.0-beta.0
Windows aarch64 build failure: https://github.com/ilan-gold/zarrs-python/actions/runs/12209833011/job/34065169380 Related: rust-cross/cargo-xwin#76. It seems like more of an issue that needs to be fixed in |
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.
Nice, mostly just little comments. I'd like to take another review though for sure once you're done with these, but looks very nice and clean. I just want to make sure I've got my head fully wrapped around it
This reverts commit 1bae158.
Agreed! Here is my comment back for posterity:
|
I had to take today off, I’ll do a real review tomorrow |
Co-authored-by: Philipp A. <flying-sheep@web.de>
OK, regarding the type hierarchy: I didn’t realize that the enum isn’t just a simple marker but contains actual data, since last time I used pyo3, it didn‘t support bridging complex enums to Python. So (if we actually exported StoreConfigType), the actual Python type hierarchy would be something like class StoreConfig: ...
class FilesystemStoreConfig(StoreConfig): ...
class HttpStoreConfig(StoreConfig): ...
class StoreConfigType:
class Filesystem(StoreConfigType, tuple[FilesystemStoreConfig]): ...
class Http(StoreConfigType, tuple[HttpStoreConfig]): ... which means that my request above wasn’t a good idea and should be reverted. After this PR, we could maybe remove class FilesystemStoreConfig:
root: str
class HttpStoreConfig:
root: str
class StoreConfig:
class Filesystem(StoreConfigType, tuple[FilesystemStoreConfig]): ...
class Http(StoreConfigType, tuple[HttpStoreConfig]): ... |
Closing and reopening to clear this Windows aarch64 pending check. Rerunning CI doesn't do it. |
|
Co-authored-by: Lachlan Deakin <ljdgit@gmail.com>
Yup, seems like it. We can’t modify them. Maybe @ilan-gold can give us the ability to do so for now? |
RemoteStore will be renamed to FsspecStore in the next beta / RC
Local benchmark
Time in second with the same datasets as https://github.com/LDeakin/zarr_benchmarks, best of 3, hot disk cache