Conversation
There was a problem hiding this comment.
Code Review
This pull request implements _cat_ranges and cat_ranges in both GCSFileSystem and ExtendedGcsFileSystem to fetch multiple byte ranges efficiently, supporting range coalescing and leveraging AsyncMultiRangeDownloader for zonal buckets. It also adds comprehensive unit tests. The review feedback suggests extracting the duplicated input validation logic into a shared helper and removing an unnecessary fallback default value of 64 for the batch size.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
==========================================
+ Coverage 90.21% 90.46% +0.25%
==========================================
Files 16 16
Lines 3749 4007 +258
==========================================
+ Hits 3382 3625 +243
- Misses 367 382 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gcbrun |
|
/gcbrun |
…m.cat_ranges Optimizes Fsspec DCP payload downloads by heavily coalescing contiguous/adjacent data chunks into singular block reads natively within _cat_ranges, preventing multi-shard GET amplification. Support includes zero-copy memoryview splicing.
…o _coalesce_ranges helper
…extended_gcsfs.py
- Fix TypeError on unbounded ranges in _coalesce_ranges when end=None - Fix batch exception scoping in ExtendedGcsFileSystem._cat_ranges - Support contiguous range coalescing when max_gap=0 - Lazily query object metadata only when unbounded ranges are present - Execute zonal and non-zonal bucket fetches concurrently - Add unit tests for middle unbounded ranges, empty inputs, mixed buckets, and partial batch failures
… lifecycle - Ensure mrd_pool is properly closed in try...finally block in _fetch_zonal_file - Move _process_limits_to_offset_and_length to GCSFileSystem and normalize limits before coalescing - Return empty bytes directly for zero-length slices without issuing I/O - Support batch_size=-1 for unchunked execution and cap effective_batch_size at 1000 - Bound concurrent zonal file download jobs with asyncio.Semaphore - Add unit tests for pool cleanup, normalization, and batch_size=-1
…ne fast path, and return types - Support tuple of paths in _validate_cat_ranges_input - Handle s is None (offset 0) in fast path without triggering unhandled _info calls - Honor caller batch_size for outer file_concurrency semaphore in ExtendedGcsFileSystem - Ensure bytes return type in ExtendedGcsFileSystem when max_gap is None - Add unit tests for tuple paths, start=None lazy info and error handling, and batch_size semaphore
…_size helpers - Centralize per-file range limit normalization, fast-path, and zero-length slice handling in GCSFileSystem._normalize_file_ranges - Eliminate duplicate normalization block in ExtendedGcsFileSystem._cat_ranges by reusing inherited _normalize_file_ranges with get_size_fn - Centralize bounded batch size computation in _compute_effective_batch_size - Add unit test for _normalize_file_ranges helper
- Merge tuple paths into test_gcsfs_cat_ranges_validation - Merge start=None normalization into test_gcsfs_cat_ranges_normalization - Merge start=None error handling into test_gcsfs_cat_ranges_error_handling - Merge zonal start=None lazy info into test_extended_gcsfs_cat_ranges_zonal_lazy_info - Merge zonal pool error handling into test_extended_gcsfs_cat_ranges_zonal_error_handling
9bf4ea9 to
799cdd1
Compare
fa21041 to
495e363
Compare
495e363 to
ef9f758
Compare
…azy info optimization
…in cat_ranges for Zonal buckets
…xtendedGcsFileSystem._cat_ranges
… and non-zonal requests
1c175e2 to
2e472db
Compare
Support auto_max_gap=True (and max_gap="auto" / "adaptive") in cat_ranges, deriving an optimal coalescing gap dynamically based on median chunk size to prevent read amplification on small ranges while optimizing throughput for large ranges.
|
@yuxin00j Can you also add pytest based before and after microbenchmarks results in PR description done over multiple runs. Looking for consistent number here for perf improvement. Lets also validate small ranges to full range read 64 kb, 5 MB file or 8 MB file and 16 MB. |
| for s, e, _ in items | ||
| if s is not None and e is not None and e > s | ||
| ] | ||
| effective_max_gap = _compute_adaptive_max_gap(all_lengths) |
There was a problem hiding this comment.
Keeping effective_max_gap can lead to regression in certain scenarios:
Example Scenario:
File A requests small 100-byte metadata slices.
File B requests 50 MB checkpoint shards.
If File B dominates, effective_max_gap scales up to 1 MB. This 1 MB gap is then applied to File A, causing up to 10,000× read amplification on small files.
If File A dominates, effective_max_gap collapses to 5 bytes, preventing legitimate coalescing on File B.
Consider making it per file
…h budget Coalescing previously had no upper bound on a merged block. A fully contiguous request set therefore collapsed into a single enormous GET regardless of max_gap (a gap of 0 satisfies every max_gap >= 0), turning a parallel fetch into a serial one. Benchmarks showed sequential reads regressing to 0.32x-0.71x of the uncoalesced baseline across zonal, regional and HNS buckets. Merged blocks are now capped at total_bytes // batch_size, which targets roughly one block per scheduler slot, bounded below by 8 MiB (below that a block transfers faster than the round trip it saves, so merging is a pure win) and above by 128 MiB. An individual range is never split, so an oversized single range still forms its own block. The cap is derived from the global byte total rather than per-file so that files contributing only a handful of ranges are still merged. Separately, the zonal MRD pool was clamped to DEFAULT_CONCURRENCY (4) while the outer scheduler releases up to batch_size batch coroutines at once, each holding an MRD for a whole batch. Large requests therefore queued behind four downloaders. The pool now defaults to the effective batch size, still bounded by the number of batches, and an explicit concurrency kwarg continues to take precedence. Also stops _coalesce_ranges from sorting the caller's list in place.
…ead zonal batches Two regressions surfaced when benchmarking the span cap. 1. cat_ranges_mixed_sizes lost its coalescing win (regional best-of 1.49x -> 1.14x, zonal 1.30x -> 0.94x). That scenario issues 200 randomly placed ranges of mixed size over a 1 GB file, so the ranges overlap heavily. Splitting an overlapping range into a new block re-fetches the bytes the two blocks share, which costs more than the parallelism the split buys. The cap now applies only to ranges that start at or after the end of the current block. 2. On zonal buckets the merged blocks are packed into batches of batch_size and each batch is downloaded over a single MRD, so coalescing shrank the batch count and left most of the pool idle. cat_ranges_seq stayed slower with coalescing on than off (0.67x-0.71x) even after the span cap. Merged blocks are now spread evenly across the pool instead of packed into the first few batches.
|
Overall range coalescing seems to be the right approach, but few points to consider:
|
Sure, I'm re-evaluating this change now as from the microbenchmark, setting default concurrency to 1 in _cat_file helps improve cat_ranges latency as well. |
Summary
This PR implements native byte range coalescing for
GCSFileSystem.cat_rangesandExtendedGcsFileSystem.cat_rangesto optimize chunk-based workload downloads (such as PyTorch Distributed Checkpoint / DCP payload loading).Adjacent and near-adjacent byte requests on the same object are coalesced into singular block reads natively, reducing request amplification and leveraging zero-copy
memoryviewslicing.Key Changes
Core Coalescing & Unpacking Helpers (
gcsfs/core.py):_coalesce_ranges: Groups and merges overlapping, contiguous (max_gap=0), and near-contiguous (gap <= max_gap) range requests for a given file, generating relative slice offsets mapped to caller indices._merge_file_ranges&_is_coalesce_enabled: Unifies conditional coalescing vs 1-to-1 range mapping._unpack_range_results: Safely populates caller results withbyteswhen coalescing is inactive (max_gap is Noneor< 0) ormemoryviewslices when active (max_gap >= 0).end=None), empty inputs, scalar broadcasting, and separate non-overlapping clusters.Auto / Dynamic max_gap Calculation (
gcsfs/core.py):_compute_adaptive_max_gap: Dynamically derives an optimal coalescing gap per file from that file's requested chunk lengths (5% of median chunk size, capped at 1 MB). Prevents read amplification on small ranges while optimizing throughput for large megabyte-scale tensors, and avoids cross-file median skew when mixing small metadata files and large tensor shards.auto_max_gap=Trueormax_gap="auto".GCSFileSystem._cat_ranges(gcsfs/core.py):asyn._run_coros_in_chunksrespectingbatch_size.max_gap,auto_max_gap=True, andmax_gap="auto"(computed per file).on_error="return"andon_error="raise"error modes.ExtendedGcsFileSystem._cat_ranges(gcsfs/extended_gcsfs.py):RAPID) buckets viaAsyncMultiRangeDownloader(MRD).pool_size = min(max_batches, max(1, concurrency))to stream multiple batches in parallel across pooled MRDs.asyn._run_coros_in_chunks._info()) only when unbounded ranges require file size resolution.MRDPoolinstances are cleanly closed infinallyblocks upon completion or failure.Testing
gcsfs/tests/test_core.py:_compute_adaptive_max_gapcovering empty inputs, small chunks, 10MB chunks, 100MB chunks (capped at 1MB), and custom ratios.cat_rangeswithauto_max_gap=True,max_gap="auto", per-file adaptive gap isolation across mixed file sizes, scalar broadcasting, contiguous coalescing (max_gap=0), multi-file coalescing, and error modes.gcsfs/tests/test_extended_gcsfs.py:auto_max_gap=Trueandmax_gap="auto").MockMRD(including auto coalesced ranges, wide uncoalesced ranges, and per-file adaptive gap isolation)._run_coros_in_chunks, return types (bytesvsmemoryview), and pool cleanup upon error.