Repository navigation
Run the batch operations of S3FileSystem concurrently (GH-1322) - #1344
Conversation
|
Paused. The design was decided in #1332: #1332 (comment) Concurrent requests here multiply to Review findings from the first
|
284b2a2 to
e669525
Compare
e669525 to
24d9872
Compare
| given = kwargs.get("max_workers") | ||
| return {**kwargs, "max_workers": min(given, window) if given else window} | ||
|
|
||
| def _run_batch( |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior) — base 7fcbcc64b02f9a5efe16ec04815b4143d1a2c615 (merge-base with master), head 24d9872ff4026dc67e6d9bd63fed6080fe11b120.
Covered: _run_batch / _batch_limit / _batch_kwargs, each batch operation (cat, cat_ranges, get, put, pipe, copy, mv, rm), the stop checks in _S3WriteSession (start, _submit, _send_part, _put_object, _complete_parts), S3File._get_range, get_file, the annotation loop, S3PathPairing.check_copy_pairs, _S3LockedCallback, and the tests.
Checked: T1 (the task pool is a separate S3ThreadPool; scopes on the request pool see a non-pool thread), the stop check in _submit sits before the request is tracked (a raise after tracking would leave abort() waiting), the completion check is inside _commit after the wait (the write becomes FAILED and abort aborts), the session's own _stop is set only by _stop_requests() (abort), and every caller of stop_retries_on() in pyathena/ (aio thread helpers, Arrow reader, primitive, writer) — none encloses an S3 write except the aio task pool (#1337).
FINDINGS (2, tests only), found by the mutation check and fixed in 24d9872f:
test_get_file_stopped_writes_no_further_blockread the same block forever, so removing the write check hung instead of failing; the stub now returns a finite sequence.- Making the single-item path of
_run_batchswallow aBaseExceptionsurvived: the interrupt test ran two items. It is now parametrized over 1 and 2 items.
After the fixes, all 23 mutants are killed.
| `on_error="omit"` or `"return"` of `cat`, `"return"` of `cat_ranges`, or `"ignore"` of | ||
| `cp` for a missing source, those errors do not stop the batch. | ||
|
|
||
| Pairs whose result would depend on the order of the transfers raise `ValueError` before |
There was a problem hiding this comment.
Self-review round 2 (claims, compatibility, operations) — base 7fcbcc64b02f9a5efe16ec04815b4143d1a2c615, head 24d9872ff4026dc67e6d9bd63fed6080fe11b120.
Claims checked against the code: cat_file and the small pipe_file send their request from the task thread (so batch_size > max_workers raises total concurrency — documented); mv deletes only after map returns; a committed multipart copy still copies its annotations under unstoppable_retries(); the conflict rules match S3PathPairing._check (self-copy kept, prefix-only skips excluded, same existing file by st_dev/st_ino, file-like by identity); rm(max_workers=) warns with stacklevel=2.
Existing callers: positional arguments of S3FileSystem.__init__ unchanged (batch_size is keyword-only after *args); AioS3FileSystem keeps its own batch_size and does not pass it to the internal filesystem; AioS3FileSystem._mv still uses _move_pairs, whose return is unchanged; no rm(max_workers=) call in pyathena/ or benchmarks/.
Operational: a stopped stop_retries_on() scope now also makes S3 reads and writes raise CancelledError before their next request (release note in the PR body). The design comment said retry_api_call and wait_with_retry_stop would reuse the new test; they keep their own snapshot of the events, which is equivalent, so that sentence is not implemented and is not claimed in the PR.
Evidence: local AWS filesystem suite 1549 passed (implementation of 24d9872f); AWS CI not run yet (Draft). CLEAN.
| counts=None if on_error == "raise" else lambda e: not isinstance(e, FileNotFoundError), | ||
| ) | ||
|
|
||
| def get( |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI (codex exec, read-only sandbox), static review — base 7fcbcc64b02f9a5efe16ec04815b4143d1a2c615, head 24d9872ff4026dc67e6d9bd63fed6080fe11b120, session 01a1b4e0-b3e5-78d1-bcf7-3b3be249ff7f. Package: detached worktree at the head, the literal diff, the #1332 specification and step 4 design, repository conventions; no PR text or prior findings. No edits, builds, tests, or GitHub access.
Coverage reported: all eight batch operations, executor/pool/scope interactions, writer and read stop placement, aio consumers, conflicts, callbacks, window arithmetic, fsspec 2026.9.0 compatibility, docs, and the 20 added test functions. No T1/T2 deadlock; stop placement, on_error, O2, O3, O4 found correct.
FINDINGS (8), all verified and fixed in 8e5bedad:
- Nonexistent local destinations differing only by case conflict on case-insensitive filesystems → compared with
os.path.normcase(os.path.realpath(...))(s3.py_check_local_destinations). get/putno longer passed**kwargsto the expansion as fsspec does (e.g.prefix=) →_copy_pairs(..., **kwargs)andLocalFileSystem.expand_path(..., **kwargs); testtest_get_put_pass_kwargs_to_expansion.- An interrupt between
_run_batch()and the cleanuptryofcatkept the outcomes → the batch runs inside thetry. copy(["b/dir", "b/x"], ["b/x", "b/y"])withb/dira bare prefix raised a conflict →conflict_candidates(copy=True)looks up every conflicting unversioned source for a copy; testtest_copy_skipped_directory_does_not_conflict.mvkeeps its rule (same input raises there too, before this PR; its contract tests assume it) — to be filed separately.- The callback proxy did not enter/exit the wrapped callback →
__enter__/__exit__delegate under the lock; the callback test now uses a branch that requires its context. get(outfile=)still checked the ignored local paths and used a task thread → it skips the check and runs in the calling thread, in order; test asserts stream contents and thread.- Docs window formula →
max(1, max_workers // n)withn = min(batch_size, objects);copydocstring too. - Docs stop guarantees → a request already sent (small PutObject) runs to its end;
cache_type="background"prefetches may still read.
Test remarks (P3) taken: the interrupt test waits for the batch's stop instead of sleeping; the mv test synchronizes both copies so one succeeds after the failure; cat_ranges on_error and the range-read stop got tests. Deferred, with reasons: the completion-timer ordering in the writer test (the mutation that removes the post-wait check is killed); a bounded driver around the writer test; an end-to-end multipart put batch (covered by the writer and batch tests separately); rm request grouping (unchanged _delete_objects, covered by its existing tests).
Validation after the repair: just lint; new mutants (copy flag, callback enter, expansion kwargs ×2, outfile thread/check, cat cleanup, stop no-op) all killed; filesystem suite 1554 passed, 1 failed (test_copy_glob_to_new_directory, during a logged credential refresh failure), which passes on rerun with its class (45 passed).
There was a problem hiding this comment.
Independent follow-up 1 (relayed): Codex CLI, static, session 01a1b506-c887-7f62-9b1b-1374c12d9cf2, range 24d9872ff4026dc67e6d9bd63fed6080fe11b120..8e5bedad2f518e07c64e13d21e6f2ca905e81060 with the earlier findings. Repairs 2, 3, 4, 6, 7, 8 confirmed. FINDINGS (3), handled in fc1a1689:
os.path.normcasedoes not fold case on macOS, so nonexistentout/Aandout/astill pass. Not fixed in code: folding case on POSIX would reject distinct files on case-sensitive filesystems. Documented indocs/filesystem.md(nonexistent local paths are compared as strings afternormcase; give destinations that differ by more than case on such filesystems).- The proxy discarded the callback that a branch returns from
__enter__()→ it now wraps that callback; the test uses a branch whose__enter__()returns another callback and checks that each branch is exited. Mutants (returningself, dropping__exit__) killed. - The
mvtest could let the second copy finish before the failure was recorded → it now waits for the batch stop (raise_if_stopped) before succeeding; a mutant that deletes after a failure is killed.
Validation: just lint; filesystem unit tests (-m "not integration") 417 passed.
There was a problem hiding this comment.
Independent follow-up 2 (relayed): Codex CLI, static, session 01a1b50e-fe7b-7dc0-af19-b75f405cd238, range 8e5bedad2f518e07c64e13d21e6f2ca905e81060..fc1a16893f. The documented case limit and the __enter__() proxy were confirmed. FINDINGS (1, test only): if the stop stopped propagating, the late copy in the mv test would fail inside its worker and the batch would keep the first PermissionError, so the test could pass without the late success. Fixed in 8d4c1b78: the worker records its success after the wait and the test asserts it. just lint; the test passes.
There was a problem hiding this comment.
Independent follow-up 3 (relayed): Codex CLI, static, session 01a1b512-8bb4-7272-a5de-9a0d5ca01734, range fc1a16893f..8d4c1b78: the mv test now requires the late success marker. CLEAN.
cat, cat_ranges, get, put, pipe with a dict, copy, and mv run their per-object calls in threads of a pool of their own, batch_size at a time, with S3Executor.map() (step 4 of the #1332 design). batch_size is an argument of these methods, rm, and S3FileSystem; None and -1 mean the request pool size. Inside a batch, each object gets max_workers // n parallel requests. A stop scope that has been stopped now also stops writers before the creation of an upload, each part (also one that waited in the pool), the completion, and single-request commits, and stops range reads and the local writes of get_file. The annotations of a committed multipart copy are still copied, retries included. Pairs whose outcome depended on the order of the sequential loop raise ValueError before any transfer. rm(max_workers=) is a deprecated alias of rm(batch_size=). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- get() and put() pass their keyword arguments to the expansion of the sources, as fsspec does. - A listed source without an object at its key, which cp_file() skips, no longer makes copy() report a conflict. - The callback proxy enters and exits the callbacks that it wraps. - get(outfile=) runs in the calling thread, in order, without checking the local paths that get_file() ignores then. - cat() drops its outcomes also when an interrupt comes before the assembly of its dict. - Local destinations are compared with os.path.normcase. - docs: the window formula with the number of objects, and the requests and background-cache prefetches that a stop does not reach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fsspec passes the callback that __enter__() returns to the transfer; the proxy now wraps that one. The docs state that local destinations that do not exist yet are compared as strings, which a case-insensitive filesystem other than Windows does not distinguish. The mv test waits for the stop of the batch before its other copy succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…est (GH-1322) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8d4c1b7 to
fb082b3
Compare
…H-1322) Its Stubber answers the requests in a fixed order, which parallel copies do not keep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if path1 == path2: | ||
| return | ||
| limit = self._batch_limit(batch_size) | ||
| source_params = self.core.copy_source_params(kwargs) |
There was a problem hiding this comment.
Rebase onto master — from base 7fcbcc64b02f9a5efe16ec04815b4143d1a2c615 (head 8d4c1b7898dd7cf145125f54cfaf6ae84746088a) onto edfafcf54a, which brought #1362 (source parameters for the lookups of copies and moves), #1369, #1370, and #1377 (authorization parameters with every request).
Conflict resolution: mv keeps the parallel copies and passes source_params to _move_pairs and the deletion; _conflict_lookups takes both source_params (upstream) and copy (this PR); copy now also sends the source's parameters to its conflict lookups, as mv does upstream. The stub of test_copy_skipped_directory_does_not_conflict accepts the new lookup_kwargs of _head_object (fb082b33).
Independent review of the rebase (relayed): Codex CLI, static, session 01a1b523-03cf-7632-a21b-d725a6a79bf1, input the git range-diff of the two series. Production resolution confirmed for both contracts. FINDINGS (1, test): upstream test_mv_source_lookup_and_delete_params answers requests in a fixed Stubber order, which parallel copies do not keep, so it could fail by scheduling. Fixed in eaa14413 with batch_size=1 in that test (the aio counterpart uses _mv, unchanged here).
Validation: filesystem suite on ed4f4311 (rebased): 1587 passed, 1 failed (the stub above, fixed); after the fixes the affected tests pass (skipped_directory, conflicts, batch: 43 passed; the mv source-parameter test three times).
There was a problem hiding this comment.
Independent follow-up of the rebase fix (relayed): Codex CLI, static, session 01a1b528-caa3-7cc0-9eef-adb65cdd526a, range fb082b33..eaa14413: batch_size=1 matches the Stubber order, and the stubbed expectations still check the source parameters, the filtered deletion, and the moved object. CLEAN.
WHAT
Step 4a of the #1332 design: the batch operations of the synchronous
S3FileSystem, built on the fan-out primitive of #1367. This branch was rebuilt on master; the earlier_run_batchwith its own refill loop is gone.cat,cat_ranges,get,put,pipewith a dict,copy, andmvrun their per-object calls (cat_file,get_file,put_file,pipe_file,cp_file,_copy_file) in threads of a pool created for the call, withS3Executor.map(), never in the request pool (T1). A single item runs in the calling thread.batch_sizeis a new argument of these methods, ofrm, and ofS3FileSystem(keyword-only). None (default) and-1meanmax_workers, the size of the request pool (O1). Other non-positive values raiseValueError.nconcurrent calls, each object getsmax_workers // nparallel requests (at least 1) through the existingmax_workerskeyword; a caller'smax_workersis resolved (None/0take the window) and capped at it (O2). A single object keeps the filesystem's window.on_errorkeeps fsspec's meaning (B5): with"omit"/"return"ofcat,"return"ofcat_ranges, and"ignore"ofcopyfor a missing source, anExceptiondoes not stop the batch; aBaseExceptionraised in a call always does.map).mvdeletes only after every copy succeeded (O3).pyathena.util.raise_if_stopped()raisesconcurrent.futures.CancelledErrorwhen an enclosingstop_retries_on()scope is set. It is checked before the creation of a multipart upload, before a part is tracked and again when it starts (a part queued behind a full pool), after the wait for the parts right before CompleteMultipartUpload (inside the commit, so the write fails and is aborted), before single-request commits (PutObject ofS3Fileandpipe_file, both CopyObject paths ofcp_file), before each range read ofS3File, and beforeget_filewrites a block locally. The annotation copies of a committed multipart copy run underunstoppable_retries().ValueErrorbefore any transfer: two pairs ofget/put/copy/pipewith the same destination after normalization, the same existing local file (e.g. hard links) or the same file-like object, and acopydestination that is another pair's source.S3PathPairing.check_copy_pairs()shares the checks ofmove_pairs(); a self-copy and a skipped prefix-only directory do not conflict.get(..., outfile=)runs one object at a time, in order.get/putand their branches are used through a lock (_S3LockedCallback), since fsspec callbacks need not be thread-safe nor their branches independent.rm(batch_size=)counts DeleteObjects requests;rm(max_workers=)is a deprecated alias (DeprecationWarning); both together raiseTypeError(O4).docs/filesystem.mddocuments the batch operations.Release notes (behavior changes):
batch_size - 1other calls are still running and stop before their next request; objects completed by other threads before the failure stay written, and their number is not bounded bybatch_size.ValueError(nonexistent local paths are compared as strings afteros.path.normcase), e.g.copy("b/d/", "b/d/sub/", recursive=True)whenb/d/sub/already holds objects.stop_retries_on()scope, reads and writes of the S3 filesystem now raiseCancelledErrorbefore their next request instead of continuing. Inside PyAthena these scopes are the step 3 primitive, the aio task pool,pyathena/aio/util.py's thread helpers, the Arrow reader, and the session's own abort.getaccepts anos.PathLikedestination for any source;copyno longer passes its copy parameters toexpand_path();getandputstill pass theirs, as fsspec does.get(outfile=)runs in the calling thread.rm(max_workers=)warns.WHY
Closes #1322 (part of #1315). Step 4a of #1332; 4b (
AioS3FileSystem, #1337) follows separately.In the s3fs comparison (#1318),
S3FileSystemran these operations 7 to 59 times slower than s3fs, because fsspec's loops call the per-object methods one after another. The design was reviewed by Codex (3 rounds) and Claude Fable 5.1 before implementation, and the maintainer decided-1= pool size,CancelledErrorfor the stop, andValueErrorfor conflicting pairs.TEST
Tested commit: eaa1441, rebased onto master edfafcf (#1362, #1369, #1370, #1377). The suite runs below are on ed4f431, the rebased series before two test-only fixes (a stub signature and
batch_size=1in an order-dependent Stubber test), which were rerun alone.just lint: passed.rumdlondocs/filesystem.md: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/(local AWS) on ed4f431: 1587 passed, 1 failed (the stub fixed in fb082b3). Before the rebase, on 8e5beda: 1554 passed.-m "not integration"): 417 passed.just benchmark test: 340 passed, 1 skipped.test_s3_writer.pyand 55 intest_s3.py(concurrency per operation,batch_sizeresolution and validation, O2 windows, failure stop, real-SIGINT drain,on_errorofcat/cat_ranges/copy, F5 of thecatassembly, O3 with a late success, conflicts, skipped prefix and in-place copies, expansion kwargs,outfile, callback serialization and contexts,rmalias, range-read and download stops, annotation retries after the stop).Not run yet: AWS CI (starts when marked Ready); the in-region measurement of #1322 (step 5 of #1332).
🤖 Generated with Claude Code