Add reader for S3 (picks up #26)

[espg]

Picks up and finishes #26 ("Add reader for S3"), ported onto current main (the reader modules have moved around quite a bit since Aug 2023, so this is a hand-port of 876cdd2 rather than a rebase).

I tried to follow the design you laid out in #26:

Making the requests async, and decompression sync is probably the best strategy when that time comes.

The performance of this reader is probably going to depend on: batching S3 requests [...]

and your sketch in #38 (comment) (index built locally / deserialized via serde, another reader on top of the existing traits and slicing code).

What this does

S3Reader in src/reader/s3.rs, behind a new s3 cargo feature (off by default):

• Same crate you picked: rust-s3 (alive and current — bumped 0.33 → 0.37, rustls kept). One deliberate change from your original commit: tokio-rustls-tls instead of sync-rustls-tls, so requests can actually be made async per your comment above. Flagging it since it swaps attohttpc for reqwest (already in the dev-deps) in the feature-gated tree.
• Async fetch, sync decompression: read_to_async issues ranged GETs with up to 16 requests in flight (buffer_unordered); decode_chunk (gzip/shuffle) runs synchronously as responses arrive.
• Request batching: chunk slices come out of group_chunk_slices sorted by file address; chunks closer than 32 KiB are coalesced into a single ranged GET (gap bytes fetched and discarded), capped at 8 MiB per request. Constants are in s3.rs, happy to tune.
• Sync Reader impl on top, so i.reader()-style usage and values::<T, _>() work unchanged: it drives the async path via Handle::try_current() + block_in_place inside a runtime, or a lazily-created static runtime outside one.
• The Bucket (region, endpoint, credentials) is configured by the caller and passed to S3Reader::with_dataset together with the object key; the reader only issues ranged GETs against it. This keeps Minio/AWS/custom endpoints out of the reader itself.

Structure follows direct.rs/cache.rs (struct shape, with_dataset, test style).

Tests

• Unit tests for the request-coalescing logic (contiguous merge, gap merge, max-size split, repeated slices of one chunk).

• Integration tests against Minio, per your benchmark-rig suggestion in Add reader for S3 #26:

  docker compose -f tests/docker-compose.minio.yml up -d
  HIDEFIX_S3_ENDPOINT=http://localhost:9000 cargo test --features s3

  They upload tests/data/{coads_climatology.nc4,dmrpp/*.h5} and compare S3Reader values against the hdf5 crate / local readers: full reads, a sliced read (exercises batching + segment copy), gzipped and shuffled chunks, and both the async (read_to_async) and the sync-inside-tokio paths. When HIDEFIX_S3_ENDPOINT is not set they skip cleanly (similar spirit to the #[ignore]d norkyst tests, but cheap enough to run whenever the endpoint is up).

cargo test without the feature is green and the compiled dependency set is unchanged; cargo fmt --check and cargo clippy --features s3 --all-targets are clean for the new code. One honest caveat on the lockfile: adding rust-s3 (even optional) forces semver-compatible bumps of a few shared locked deps (serde 1.0.210→1.0.228 which now splits out serde_core, syn, libc — sysinfo from the rust-s3 chain requires the newer syn/libc). The default-feature dependency set is otherwise identical.

Not benchmarked against real S3 — local Minio has none of the latency that makes the batching/concurrency constants matter, so the Thredds-vs-Minio benchmark from #26 is still open.

Small rider

One extra line in .github/workflows/python.yml: a ubuntu-24.04-arm entry in the wheel matrix, so tagged releases also publish linux aarch64 wheels (PyPI currently has none for that platform — relevant for Graviton/Lambda users like us).

Questions for review

1. Your original stub imported ParReader too; I left rayon-parallel reads out since request concurrency already overlaps the fetches and the decode is cheap relative to network. Want a ParReader/Streamer impl as a follow-up, or in this PR?
2. CONCURRENT_REQUESTS/COALESCE_GAP/MAX_REQUEST_SZ are consts; fine, or would you rather have them configurable on S3Reader?
3. No python-binding exposure in this PR — worth a follow-up once the crate-level API settles?

[espg]

@gauteh I'm looking for a light compiled option that's faster than and can replace h5coro . Did some initial benchmarking, and hidefix works really well, almost as fast as the C++ bindings, but with much less dependencies to manage and package:

variant	o10 read wall	o9 read wall	peak RSS (o10/o9)
h5coro 1.0.4 (baseline)	93.5 s	129.0 s	248 / 282 MB
h5coro + numpy shuffle patch (patches/)	24.5 s (3.8×)	34.0 s (3.8×)	255 / 286 MB
hidefix 0.12.0 (Rust, incl. per-granule index build)	15.5 s (6.0×)	20.8 s (6.2×)	155 / 161 MB
sliderule C++ H5Coro (pybind shim)	10.3 s (9.1×)	14.2 s (9.1×)	187 / 208 MB

This PR was mostly done using the new Fable / Mythos anthropic model (before they start charging for it next week), which seems to be doing pretty well so far. Let me know if you have any questions or have followup asks.

[gauteh]

Thanks! Very interesting!

Just a couple of comments as they pop into my head:

• Minimizing the number of requests probably speeds things up: so that if you have multiple chunk separated with a single chunk it is probably better to get the whole range.
• Do you need a cache, similar to StreamReader, to prevent fetching the same chunk multiple times (in case a hyperslice passes through the same chunk in a non-contiguous way).
• Usually the direct Reader is fastsest, your current solutions is practical, and I assume the s3-lib requires tokio to work? Otherwise, if it does support blocking reads maybe its better to use those if creating a blocking reader? Or will you loose concurrent network reads then?
• In the other readers decompression is by far the greatest bottleneck. I can imagine this becomes an issue when using local network S3 servers. Parallelization of decompression was the biggest gain there.

All in all, I think this can be merged and then it can be extended later. Thanks for keeping it small and incremental when using AI, otherwise hard to review.

I see some tests are failing, I bet Mythos/Fable can figure those out quickly :)

[espg]

Thanks for the quick look! Point by point:

• More aggressive coalescing — agreed. Right now the merge rule is a fixed byte gap: chunks are coalesced when the hole between them is ≤ COALESCE_GAP (32 KiB, fetched and discarded), capped at MAX_REQUEST_SZ (8 MiB). So an intervening chunk is already absorbed when it's small; for larger chunks the right knob is making the gap threshold relative (e.g. allow holes up to about one chunk's stored size, or a fraction of the request cap) instead of a constant. The request/offset bookkeeping already handles arbitrary holes, so it's a one-line predicate change — I'd rather tune it against a real endpoint with benchmarks (latency saved vs bytes discarded) as a follow-up.

• Cache / duplicate fetches — within a single read there's no duplicate fetch or decode: group_chunk_slices returns slices sorted by file address, so all slices of one chunk are adjacent, and group_requests folds them into a single entry per chunk (the group_slices_of_same_chunk unit test covers this). Each chunk is fetched once, decoded once, and copied out to each destination segment. Across separate read calls there's no cache — an LRU keyed on chunk address like StreamReader's ds_cache would bolt on cleanly, good follow-up.

• tokio / blocking reads — rust-s3 does have a blocking mode (the sync feature swaps reqwest for attohttpc via maybe-async), so a blocking reader is possible, but it would serialize the GETs — and the concurrent in-flight requests (buffer_unordered, 16 at a time) are most of the win against object-store latency. That's the same conclusion as your note on Add reader for S3 #26 ("making the requests async, and decompression sync"). Since tokio is already a non-optional dependency of hidefix, the async route adds no new runtime requirement; the sync Reader::read_to just drives the async path on a shared runtime.

• Parallel decompression — agreed, and it's currently the serial part: decode happens inline as responses land. Moving it onto rayon like direct.rs does (par_iter over chunk groups) is the natural next step, and exactly for the local-network/high-bandwidth case you describe. I'd take that as the first follow-up after this lands.

[gauteh]

Ok. Sounds good. Hopefully the tests can be fixed, I suspect most of the errors are due to outdated code / CI and not this PR. But it would be nice to see things compile to be sure that the new changes do not break things. Feel free to update dependencies also across major versions.

[gauteh]

Thanks, merged. The other errors are unrelated to this PR.