First pass at python bindings for zarr store EAR-1148 #39
Conversation
| name: wheels-sdist | ||
| path: dist | ||
|
|
||
| release: |
There was a problem hiding this comment.
maybe we comment out this to avoid mistakes before we are ready to release.
There was a problem hiding this comment.
This is in the subfolder so it won't run until it's moved to the root I believe. I'll confirm tho
icechunk-python/src/lib.rs
Outdated
| key_ranges: Vec<(String, ByteRange)>, | ||
| ) -> RustStoreResult<Vec<Option<Vec<u8>>>> { | ||
| // Not sure how to hanlde ARC requirement yet | ||
| //let result = self.store.get_partial_values(&key, key_ranges.iter()).await?; |
There was a problem hiding this comment.
@paraseba I am not sure the best way to handle this function, since self on the zarr store expects an Arc. Any ideas? If i want to wrap the inner store in an Arc then i need a mutex for it to mutable, but then that messes with it being an arc haha
There was a problem hiding this comment.
Also since python expections are not values, they are top level only, so we cant pass a vactor of them. So we have a choice between bubbling errors to the top or ignoring inner errors and returning None for keys that fail. The zarr spec says keys that dont exist return None but thats maybe not comprehensive
| @@ -0,0 +1,72 @@ | |||
| /target | |||
There was a problem hiding this comment.
this is a bit more comprehensive that we may need? Would it make sense to add a few lines to the .ignore file at the root instead?
There was a problem hiding this comment.
This can be cleaned up.. this is what is autogenned from the maturin project template
icechunk-python/src/errors.rs
Outdated
| use pyo3::{exceptions::PyValueError, PyErr}; | ||
|
|
||
| /// A simple wrapper around the StoreError to make it easier to convert to a PyErr | ||
| pub struct RustStoreError(pub StoreError); |
There was a problem hiding this comment.
we are using thiserror crate for this kind o things and it works very well. It gives you Error implementations, including the display parts and the error sourcing.
There was a problem hiding this comment.
We need this in order to map theStoreErrorto PyError, which is a python exception. We cannot impl the From<PyError> on StoreError in this crate because of orphan rules
| // and the lifetime of the stream is tied to the lifetime of the store. So the return | ||
| // value is valid as long as the store is valid. Rust has no way to express this to | ||
| // the Python runtime so we have to do it manually. | ||
| fn pin_extend_stream<'a>( |
There was a problem hiding this comment.
Really happy we have a way to do this, even if it's unsafe.
Can we find a way to make this less generic on 'a by tying it to the lifetime of IcechunkStore. This is really lying right? If IcechunkStore is dropped, this value will fail, it's not really 'static for example.
There was a problem hiding this comment.
Correct this is lying. The reason is that python PyClass wrpapers have no support for lifetime annotations, so you cannot enforce the lifetimes. Its really hacky and there is no real other way to manage it that i can think of unless we change downstreamn implementation to not be bound to the lifetime of the downstream store
mpiannucci
changed the title
EAR-1148 Develop Python interface to Icechunk v2
Icechunk has a async Zarr store-like interface in We need to build an async python Zarr store that can talk to this async Rust interface. Some links: |
mpiannucci
force-pushed
the
python-bindings
branch
from
736a2c4 to
9ff2bd3
Compare
Putting this up just for visibility as i figure out the most efficient way to bind errors and results