-
Notifications
You must be signed in to change notification settings - Fork 0
feat: get partials #51
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abidsikder
left a comment
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 see that this PR is waiting for kubo 0.36. In the meantime, perhaps we can consider using python's native slice() type for the byte range request parameters in KuboCAS?
|
you mean for InMemoryCAS where i do |
I meant for the input arguments. I was suggesting they take a python slice() object instead. But I'm no longer sure that makes much sense for just an offset and length, if that's what zarr encrypted store is using and is more compatible with the concept of byte ranges. Perhaps we can disregard this suggestion then. |
|
@TheGreatAlgo were you able to test this out? master-latest docker image described here https://github.com/ipfs/kubo?tab=readme-ov-file#-developer-preview-images from https://hub.docker.com/r/ipfs/kubo/tags?name=master-latest should have the new fixed gateway work in it. Could just be a matter of running that docker image locally, pinning a dataset like the cpc one, and then testing if the get partials works against the gateway for the local running docker node. As a sidenote, nothing important here just referencing the fsspec one which this PR appears to implement pattern wise https://github.com/zarr-developers/zarr-python/blob/d19f3f0a825b5be986fa6e2f797aa4ddec0b90ed/src/zarr/storage/_fsspec.py#L309
|
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.
Great tests!
Only thing I would suggest is to refactor the random xarray Dataset generation into a pytest fixture since it's being used across multiple tests now, but this PR seems ready to merge as is.
|
being done on sharding |






You can test by pulling the latest kubo node as a docker container
https://github.com/ipfs/kubo?tab=readme-ov-file#-developer-preview-images
Then you need to remove the rpc_base_url and gateway to use local defaults. That way it connects to docker kubo node running 0.36. Its otherwise not officially release.

I do have full test coverage, so if you don't want to do that, you can refer to this image.
