feat: Handle all forms of range requests in fsspec - #766
Conversation
| class _CoalesceKwarg(TypedDict, total=False): | ||
| """The optional `coalesce` argument of [obstore.get_ranges][].""" | ||
|
|
||
| coalesce: int |
There was a problem hiding this comment.
This seems unnecessarily complicated when you could just add a parameter into a dict
There was a problem hiding this comment.
Agree on _CoalesceKwarg being ugly, it was added solely to satisfy the type checker. To make your suggestion work, I had to pass lengths explicitly, otherwise Pyright would complain.
| def _needs_object_size(start: int | None, end: int | None) -> bool: | ||
| """Whether resolving a range requires knowing the size of the object.""" | ||
| return end is not None and (end < 0 or (start is not None and start < 0)) |
There was a problem hiding this comment.
Resolving a range should never need to know the size of the object (except on Azure, which doesn't support suffix requests).
I'd strongly recommend taking a similar approach to the Zarr-Python obstore adapter. https://github.com/zarr-developers/zarr-python/blob/d44f9f92ab4f12a8008de2553a7c9988669e3910/src/zarr/storage/_obstore.py#L440-L491
(For Azure, you can have a config parameter for the fsspec adapter for whether to avoid suffix requests, which then would make a HEAD request to always know the size)
There was a problem hiding this comment.
There are two cases where we need to know the size of the object: when start is negative and the backend does not support suffix requests, and when the bounds do not map to an HTTP range request, which occurs if end is negative, or when a negative start is paired with an end.
For the first case, I went with the zarr adapter's runtime fallback instead of a config parameter, which looks up the size after the store refuses the suffix.
| starts: Sequence[int | None] | int | None, | ||
| ends: Sequence[int | None] | int | None, |
There was a problem hiding this comment.
Can you link to source code in fsspec that supports this typing? I.e. are there tests or a code path where we know that starts and ends can take None, either in isolation or as an element in a sequence?
There was a problem hiding this comment.
Added a comment with a permalink in _cat_ranges.
kylebarron
left a comment
There was a problem hiding this comment.
Overall this PR still seems to be overly complex for what it does (I'd say it seems to be written too much by Claude), and needs to be simplified a lot before merge
|
Context from a Claude review on my side, if you're interested. From a quick read of this summary, I think all of these are valid points that would need to be addressed before merging. Thanks for the rework — going the adapter-only route is right, and the overall shape is what I had in mind: bounded ranges batched per object through I built this branch and ran the suite against minio: 28 passed, 1 xfail, and all the new tests pass. ruff is clean; pyright shows only the pre-existing missing-stub errors for the Some follow-ups on my inline comments, including one where I was wrong. Follow-ups on the inline comments
|
|
Thanks for your detailed feedback, this is all fair. Everything you raised is implemented locally. Regarding complexity, I'm afraid the PR has not gotten smaller after incorporating your points, but I think the code quality has improved. I still need some time to check whether everything is consistent now, and whether I can simplify it any further. Will ping when ready for another look. |
|
I've addressed all the points you raised and updated the PR description. |
Closes #259.
Adds support for all forms of range requests documented by fsspec, without changing obstore's own API.
Implemented:
cat_fileandcat_rangesaccept every range form fsspec documents: either bound alone, and either counting back from the end of the object.cat_rangesadditionally takes a scalar orNonebroadcast across all paths, andNoneelements. It also honorsmax_gap,batch_size, andon_error, which were previously ignored.on_error="raise", failures now come back in the returned list.get.test_cat_ranges_mixedis no longerxfail.Request cost: A plain start/end goes through
get_range;start-only and negative-startbecomegetwith{"offset": n}and{"suffix": n}, so they stay single requests. A zero or absent start with no end sends no range header.cat_rangessplits its input the same way_get_partial_valuesdoes in the zarr PR: bounded ranges batch per object throughget_rangesso nearby ones are merged, and each open-ended range is a separategetrequest. The object size is only needed for a negativeend, or a negativestartpaired with anend.Degenerate ranges: Zero-length, inverted, and
start-past-the-end ranges raise an exception rather than returning empty. The first two are rejected byvalidate_rangeinobstore/src/get.rs, the third byobject_storeitself.This only touches
fsspec.pyand its tests.get_rangeandget_rangesare unchanged.