Repository navigation
s3 improvements: client cache and batch transfers - #112
Open
karank2512 wants to merge 4 commits into
Open
karank2512 wants to merge 4 commits into
karank2512 wants to merge 4 commits into
Conversation
…h transfers
The S3 layer (thaw_common.cloud) rebuilt a fresh boto3 client on every
download and every upload, and had no API for moving many objects at once.
Building a client is ~3 ms warm / ~hundreds of ms cold (credential chain +
S3 service-model load) and — the part that bites on a real network — starts
with an empty connection pool, so each operation paid a fresh TCP+TLS
handshake instead of reusing a warm keep-alive connection. And per
docs/ARCHITECTURE.md a single S3 key is server-throttled to ~135 MB/s flat
regardless of ranged concurrency; the documented unlock is fanning out
across N keys (N×135 MB/s) — which there was no primitive for.
Changes (all additive; existing public APIs and behavior unchanged):
* Shared, thread-safe, process-cached boto3 client keyed by (pid, pool_size).
- pid key: botocore clients are not fork-safe; vLLM forks/spawns TP
workers, so re-keying on os.getpid() rebuilds transparently per process.
- pool_size key: callers needing a bigger HTTP pool get a distinct client.
- Lazy, double-checked build under a lock (boto3.client() construction is
itself not thread-safe; the instance is). reset_s3_client_cache() hook
for test isolation / operator recovery.
* resolve_snapshots(uris, ...) / upload_snapshots(pairs, ...): bounded
cross-file fan-out (THAW_S3_FILE_CONCURRENCY, default 8) reusing the shared
client. Order-preserving, first-error-surfacing, atomic .part rename per
file preserved. resolve_snapshots de-duplicates identical URIs so two
positions can't race the same cache file's rename.
* Honest docstring: replace the unreceipted ">800 MB/s" claim with the
receipted ~135 MB/s single-key wall + the many-key fan-out lever.
Tests: +20 unit tests (client reuse, fork rebuild, reset, batch ordering,
concurrency, bounds, error propagation, dedup) + 3 moto end-to-end batch
tests. Full non-GPU suite: 273 passed. Benchmark (benchmarks/bench_s3_batch.py,
receipt in site/receipts/): client construction 2.83 ms->0.025 ms per op;
256 objects @ 40 ms -> 31.6x with 32-way file concurrency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
perf(cloud): cache the boto3 S3 client + add bounded-concurrency batc…
… MB/s claim, scrub local path from receipt, drop unmeasured connection-warmth and hard-bound claims)
…nged files is clean
|
@karank2512 is attempting to deploy a commit to the Nils 's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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. Comment |
Closed
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continues #93 (same change, moved to a feature branch so the fork's
mainstays clean). Addresses the review there.What it does
(pid, pool_size).resolve_snapshots/upload_snapshots: bounded cross-file concurrency, input order preserved, duplicate URIs deduplicated before fan-out.benchmarks/bench_s3_batch.pyand receiptsite/receipts/2026-06-24_s3_batch.json, tests intests/test_cloud.py.Review feedback addressed
cloud.pyno longer say N keys "give" ~N x 135 MB/s. They say throughput should scale toward it, projected from the single-key ceiling and not yet measured end to end.json_outinto new receipts.main; the README S3 section is reconciled with the shortened version there.Checks
python tools/thaw_lint.pyclean; ruff (F,E9) clean on changed files;pytest tests/293 passed with CI's dependencies.Known, not fixed here
.partname and there is no size check, so a corrupt snapshot is possible (pre-existing, more likely with batching).docs/ARCHITECTURE.mdciteslegacy-site/receipts/2026-04-23_ec2_s3_download.jsonfor the 135 MB/s figure, and that file is not in the tree at present.