Repository navigation
Keep multipart uploads within the 10,000-part limit - #968
Conversation
| # Abort the upload and close the file without the | ||
| # buffered data, so that neither close() nor commit() | ||
| # uploads it. | ||
| self.discard() |
There was a problem hiding this comment.
Self-review round one (behavior and implementation) — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 692167df5d21928f908bc2d00aeca573a899d51f. Result: FINDINGS (F1, F2; both repaired in a follow-up commit, see reply).
Covered: S3File._upload_chunk limit check (sync S3File and AioS3File, which inherits it), fsspec 2026.9.0 flush()/close() flow when _upload_chunk raises (mid-stream write() and final close()), deferred commit() and transaction rollback through discard(), append copied parts counting toward the limit, _get_copy_ranges callers (sync/async _copy_object_with_multipart_upload, append path), docs, and the tests.
F1 — an abort failure masks the limit error. pyathena/filesystem/s3.py:2379: self.discard() runs before self.buffer = None / self.closed = True. If AbortMultipartUpload fails, its error propagates instead of the ValueError, the file stays open with its buffer, and close() flushes again, reaching the check and retrying the abort. Reproduced with a mocked filesystem whose _call raises PermissionError: the caller saw PermissionError: abort failed and the abort was called twice.
F2 — after an abort failure, a deferred commit() completes a partial object. Same line: discard() raises before clearing multipart_upload_parts, so with autocommit=False a later commit() (e.g., a transaction that swallowed the error) takes the _finish_multipart_upload branch and completes the first 10,000 parts. Verified by the extended regression test failing without the repair.
Repair: close the file and drop the buffer first, log an abort failure as _finish_multipart_upload does, clear the upload state on that failure as commit() does after its own abort, and always raise the ValueError.
There was a problem hiding this comment.
Repaired F1 and F2 in ff6de77: the file is closed and its buffer dropped before discard(); an abort failure is logged and clears multipart_upload/multipart_upload_parts (as commit() does after its own abort); the ValueError is always raised. New test_write_exceeding_max_parts_abort_failure[True/False] fails without the repair (PermissionError for autocommit=True's sibling scenario, and a completed upload after commit() for autocommit=False) and passes with it. just lint passes; offline tests/pyathena/filesystem/: 163 passed, the 108 credential-less integration failures are the same set as on the base.
| The ``(start, end)`` byte ranges, with an exclusive end, that | ||
| cover the whole object in order. | ||
| """ | ||
| block_size = max(block_size, math.ceil(size / self.MULTIPART_UPLOAD_MAX_PARTS)) |
There was a problem hiding this comment.
Self-review round one — copy split size checked, no finding: math.ceil(size / 10_000) is exact for sizes within the 5 TiB object limit (below 2**53), stays ≥ MULTIPART_UPLOAD_MIN_PART_SIZE because the validated block_size is the lower bound, and stays ≤ about 550 MB, so a merged short tail never needs the half split and the count stays ≤ 10,000. The async copy reaches it through self._sync_fs._get_copy_ranges.
| f"Cannot upload more than {self.fs.MULTIPART_UPLOAD_MAX_PARTS} " | ||
| f"parts to s3://{self.bucket}/{self.key} with a block size of " | ||
| f"{self.blocksize} bytes. Write the file with a block_size, or " | ||
| "a default_block_size of the filesystem, of at least its total " |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations) — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head ff6de7757fd1a946ddc37c0d8024bb7ba17356ba. Result: FINDINGS (F3; repaired, see reply).
F3 — the block size guidance is not enough for an append. pyathena/filesystem/s3.py:2394 (and the docs formula): "at least its total size divided by 10,000" holds for a plain write, but an append to an object below 5 GiB copies it as one extra part. With block = T/10,000 and an existing object E with E + 5 MiB ≤ block, the appended data takes 9,999 full blocks plus a tail of at least 5 MiB, i.e. 10,000 parts plus the copy = 10,001 (e.g., appending ~200 GiB to a 5 MiB object). Reproduced scaled down (limit 10, existing 4 B, appended 196 B, block 20): ValueError. Larger existing objects keep enough slack (copied parts ≤ E / 5 GiB + 1 versus 10,000·E/T ≥ E / 550 MB). Repair: the message asks for a block size large enough to fit in 10,000 parts, including copied parts; the docs keep the formula for writes and state that an append's copied parts count toward the limit.
There was a problem hiding this comment.
Repaired F3 in 41abb14: the error now asks for a block_size (or default_block_size) "large enough for it to fit in 10000 parts, including the parts copied from the existing object in an append", and docs/filesystem.md adds "In an append, the parts copied from the existing object also count toward the limit." just lint, just docs lint, and the offline max_parts tests (18) pass.
| [fsspec transaction](https://filesystem-spec.readthedocs.io/en/latest/features.html#transactions), | ||
| writes are deferred until the transaction commits and are discarded on rollback. | ||
|
|
||
| A multipart upload consists of at most 10,000 parts, one per block, so a write with |
There was a problem hiding this comment.
Self-review round two — claims checked against evidence:
- 1 to 10,000 part numbers: botocore S3 model docs for
UploadPart/UploadPartCopyPartNumber ("a positive integer between 1 and 10,000"). - ~48.8 GiB = 10,000 × 5 MiB at
DEFAULT_BLOCK_SIZE; parts are one per block except a merged short tail (_upload_chunklookahead). block_sizeofpipereachesopen(): fsspec 2026.9.0pipe→ PyAthenapipe_file→ fsspecpipe_file→self.open(path, "wb", **kwargs).put/put_fileopens withoutblock_size, sodefault_block_sizeis the documented way there (also forAioS3FileSystem._put_file, which delegates to the syncput_file).cpcopies stay within the limit:cp_file→_copy_object_with_multipart_upload(> 5 GiB only) →_get_copy_ranges; same for the async copy.- Caller compatibility: the new exception is
ValueError(release-noted);buffer: BytesIO | Noneis an annotation only. - Operations: the error comes after the first 10,000 parts are uploaded (release note updated); the abort is a single request through
_call's retry config; a failed abort is logged and the incomplete upload stays listable/clearable (list_multipart_uploads/clear_multipart_uploads). - Deferred (pre-existing
discard()behavior, not introduced here): part uploads already running when the abort is sent may still complete. - Evidence: local offline tests only; live AWS coverage comes from CI once Ready. PR body corrected (message wording, release note timing, tested commit).
| # upload. An abort failure does not mask this error, and | ||
| # commit() does not complete the upload afterwards. | ||
| self.buffer = None | ||
| self.closed = True |
There was a problem hiding this comment.
Independent review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100b3-237e-7d81-81b1-935b1d6ce6d7. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 41abb14fde412a6a81dd20d54201db3875cac369, detached snapshot; given the diff, the intended behavior, and repository conventions only (no PR text, commit messages, or prior findings). Static review only: no builds, tests, or network access. Result: FINDINGS (3).
Covered (reviewer): the supplied diff, sync/async write and copy callers, append accounting, fsspec 2026.9.0 buffering and transactions, abort handling, executor lifecycle, typing, documentation, and tests. The reviewer confirmed that the per-part guard covers multi-block writes, merged/split tails, and copied append parts; that clearing the state prevents later publication, including after an abort exception; that copy ranges satisfy count, size, and coverage bounds through 5 TiB; and that no supported flush path exposes None through the cast.
1. P2 — Limit failures can leave the executor running. (reviewer text, condensed) With autocommit=False, write 10,000 full blocks followed by one byte, then call close(). The final flush raises ValueError, so execution never reaches _executor.shutdown() at pyathena/filesystem/s3.py:2286; the file is already marked closed, so __del__ skips cleanup. A direct mid-stream write() failure also closes the file without shutting down its executor. The new failure path introduces this exposure; close() lacking exception-safe shutdown is pre-existing.
Author verification: confirmed for the final-flush path (super().close() raises before shutdown()); a mid-stream failure followed by close() reaches shutdown() because super().close() returns early on a closed file. Pre-existing for other final-flush errors; folded as a contained fix.
There was a problem hiding this comment.
Repaired in b8224b1: S3File.close() shuts the executor down in a finally block, so a failing final flush (the part limit error or any other) no longer skips it. The new executor.shutdown.assert_called_once() in test_write_exceeding_max_parts fails for the close()-path cases without the repair and passes with it.
There was a problem hiding this comment.
Self-review of the repair b8224b1 (both perspectives, range 41abb14f..b8224b19, same base). Behavior: the try/finally in close() does not change which exception propagates; S3AioExecutor.shutdown() is a no-op and S3ThreadPoolExecutor.shutdown() behaves as on a normal close; a mid-stream error followed by close() reaches shutdown() as before. Claims: put_file writes remote.blocksize chunks (one part per block) and pipe writes once (blocks plus a merged tail), so the docs formula holds for them; the open sentence says "can" because a remainder below the minimum part size is merged. Offline tests/pyathena/filesystem/: 163 passed, the same 108 credential-less integration failures as the base. Result: CLEAN.
| f.commit() | ||
|
|
||
| assert f.closed | ||
| assert fs._upload_part_copy.call_count + fs._upload_part.call_count == 3 |
There was a problem hiding this comment.
Independent review (relayed), finding 2. P2 — Failure tests have scheduling-dependent assertions. (reviewer text, condensed) tests/pyathena/filesystem/test_s3.py:2053 and :2076 use a real thread pool but require exactly three mocked upload calls. If a submitted future is still queued when the fourth part triggers the guard, discard() cancels it before the mock runs, so correct behavior can produce fewer than three calls. Record the submitted part numbers deterministically. These tests also omit executor-shutdown assertions.
Author verification: confirmed (discard() cancels futures that have not started).
There was a problem hiding this comment.
Repaired in b8224b1: the failure tests pass a MagicMock(wraps=S3ThreadPoolExecutor(max_workers=1)) executor and assert the submitted part numbers ([1, 2, 3]) and one shutdown instead of the executed upload count. 30 repeated runs of the max_parts tests: 30/30 passed.
|
|
||
| A multipart upload consists of at most 10,000 parts, one per block, so a write with | ||
| the default block size can upload up to about 48.8 GiB (10,000 × 5 MiB). To write a | ||
| larger object, use a block size of at least its size divided by 10,000, either with |
There was a problem hiding this comment.
Independent review (relayed), finding 3. P3 — The documented block-size formula is insufficient for arbitrary streamed writes. (reviewer text, condensed) For 5,001 writes of 15 MiB each with block_size=10 MiB, the block size exceeds total/10,000, yet each flush emits separate 10 MiB and 5 MiB parts, so the 5,001st write exceeds the limit. Qualify the guidance for write boundaries and partial blocks. The inaccurate guidance is introduced here; the flush behavior is pre-existing.
Author verification: confirmed by _upload_chunk (a remainder of at least the minimum part size after the last full block is not merged). put_file writes in blocksize chunks and pipe writes once, so the formula holds for them.
There was a problem hiding this comment.
Repaired in b8224b1: the docs now say put and pipe upload one part per block (so the formula applies to them), and that a file written with open can take more parts because each write that fills the buffer uploads its data beyond the last full block as a separate part. just docs lint passes.
| (10,000 × 5 MiB). To upload a larger object, use a block size of at least its size | ||
| divided by 10,000, either with the `block_size` argument of `pipe` and `open` or with | ||
| the `default_block_size` argument of `S3FileSystem`. A file written with `open` can | ||
| take more parts, because each write that fills the buffer uploads the data beyond its |
There was a problem hiding this comment.
Independent follow-up review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100bb-134a-7271-a141-01de05fb7e73. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head 41abb14fde412a6a81dd20d54201db3875cac369, new head b8224b1957948918124f30c5359129c5c6fe783e; given the incremental diff, the intended behavior, and the affected contracts only. Static review only. Result: FINDINGS (1).
Covered (reviewer): incremental diff and base-to-head context; S3File.close(), _upload_chunk(), commit/discard; inherited AioS3File.close() and both executors; fsspec 2026.9.0 close/flush/__del__; put_file/pipe_file; both named tests and the docs. No actionable defects in the shutdown repair or the revised test assertions.
P3 — Missing minimum-size condition for separate remainder parts (introduced by the incremental diff). With the default 5 MiB block size, a 6 MiB write submits one 6 MiB part: _upload_chunk() merges the 1 MiB remainder into the preceding block, so "each write that fills the buffer uploads its remainder separately" gives wrong part counts. Qualify it with the minimum part size.
Author verification: confirmed (0 < next_data_size < MULTIPART_UPLOAD_MIN_PART_SIZE merges).
There was a problem hiding this comment.
Repaired in 8b99abb: the sentence now ends "as a separate part when that data is at least 5 MiB". Self-review of the wording (claims: matches _upload_chunk's merge condition with MULTIPART_UPLOAD_MIN_PART_SIZE = 5 MiB; behavior: docs only). just docs lint and just lint pass.
There was a problem hiding this comment.
Independent follow-up review (relayed) of the repair — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100bf-f35e-7521-ab60-c3e46b71311c. Previous head b8224b1957948918124f30c5359129c5c6fe783e, new head 8b99abbb7612b1594263d626b75f7b79a51d5cc7, base f3ef301d9fec92f1fc53b147bebfef87bfc919ca. Static review only. Covered: the incremental diff and the full paragraph; _upload_chunk, the multipart constants, put_file/pipe_file, append accounting, the overflow abort, and copy sizing; fsspec 2026.9.0 write/flush, buffer reset, close, and pipe delegation. Result: CLEAN — the "at least 5 MiB" condition matches the implementation (e.g., with a 10 MiB block size, a buffered 12 MiB write produces one merged part; 15 MiB produces separate 10 MiB and 5 MiB parts).
| starts.append(starts[-1] + (size - starts[-1]) // 2) | ||
| return list(zip(starts, [*starts[1:], size], strict=True)) | ||
|
|
||
| def _check_multipart_upload_size(self, path: str, size: int, block_size: int) -> None: |
There was a problem hiding this comment.
Self-review rounds one and two of the pre-check extension — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, range 8b99abbb7612b1594263d626b75f7b79a51d5cc7..6901304bea3cb5112903407b4b7d5d78eb50ed38 (a contract expansion requested by the maintainer after consulting claude-fable-5-1). Result: CLEAN.
Round one (behavior): the check runs before parse_path-dependent work, exists() (mode="create"), any API call, and the transaction branch of pipe_file, so nothing is opened or added to transaction.files; sizes up to one block never reach it; an empty file passes; AioS3FileSystem._put_file/_pipe_file delegate to the sync methods; put_file passes block_size to open(), where S3File.__init__ still rejects a write block size below 5 MiB. New tests fail without the source change (8 cases) and pass with it; offline tests/pyathena/filesystem/: 171 passed, the same 108 credential-less integration failures as the base.
Round two (claims, callers): size > block_size * MULTIPART_UPLOAD_MAX_PARTS is exact for put_file (it writes remote.blocksize chunks, so parts = ceil(size / block)) and is the documented rule for pipe_file, which therefore rejects the sizes in (10,000·B, 10,000·B + 5 MiB) that the tail merge would have fit (release-note). The minimum block size in the message is max(ceil(size / 10,000), 5 MiB), so it never suggests a size open() rejects. put(…, block_size=…) previously reached the S3 API as a parameter and failed, so accepting it breaks no caller. Pre-existing and out of scope: put_file still forwards max_workers to the S3 API.
| # neither close() nor commit() uploads it, and abort the | ||
| # upload. An abort failure does not mask this error, and | ||
| # commit() does not complete the upload afterwards. | ||
| self.buffer = None |
There was a problem hiding this comment.
Independent review of the expanded scope (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100db-fc47-7d52-93c8-beff25916d64. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 6901304bea3cb5112903407b4b7d5d78eb50ed38 (full diff, detached snapshot; intended behavior and conventions only). Static review only. Result: FINDINGS (2). Covered (reviewer): the supplied diff, sync/async upload paths, fsspec 2026.9.0 lifecycle and transactions, append accounting, merged/split tails, copy-range bounds through 5 TiB, pre-check placement, typing, documentation, and tests.
1. P2 — Mid-stream overflow leaves the executor running (introduced). Outside a context manager, writing past the limit sets closed=True, drops the buffer, and aborts, but never shuts down the executor; commit()/discard() do not, and fsspec's __del__() skips closed files. The tests masked it because their helper always exits a context manager.
Author verification: confirmed (fsspec __del__: if not self.closed: self.close()).
There was a problem hiding this comment.
Repaired in da55411: the error path calls self._executor.shutdown() after the abort (idempotent with close(); S3AioExecutor.shutdown() is a no-op). New test_write_exceeding_max_parts_without_close (no context manager) fails without the repair and passes with it; the existing failure tests now assert shutdown was called rather than called once. Self-review of the repair (behavior: waits for in-flight parts like close(); claims: fsspec __del__ skips closed files) found nothing further.
| ``MULTIPART_UPLOAD_MAX_PARTS`` blocks. | ||
| """ | ||
| block_size = kwargs.get("block_size") or self.default_block_size | ||
| self._check_multipart_upload_size(path, len(value), block_size) |
There was a problem hiding this comment.
Independent review of the expanded scope (relayed), finding 2. P2 — The new pre-check undercounts memoryviews (gap in the added check; the length-based dispatch predates the diff). len(memoryview) counts items: with B = 5 MiB, a four-byte-item view of 10,000·B + 4 bytes passes the check, and its tail merges into part 10,000, so nothing rejects it. Use memoryview.nbytes.
Author verification: confirmed (len(memoryview(array('I', [0] * 4))) is 4, nbytes is 16).
There was a problem hiding this comment.
Repaired in da55411: pipe_file() computes size = memoryview(value).nbytes once and uses it for both the part limit check and the PutObject dispatch (the dispatch had the same pre-existing undercount). New case memoryview(b"a" * 16).cast("I") in test_pipe_file_exceeding_max_parts fails without the repair and passes with it. Offline tests/pyathena/filesystem/: 173 passed, the same 108 credential-less integration failures as the base; just lint passes.
| # The size in bytes; the length of a memoryview counts its items. | ||
| size = memoryview(value).nbytes | ||
| self._check_multipart_upload_size(path, size, block_size) | ||
| if self._intrans or size > min(block_size, self.MULTIPART_UPLOAD_MAX_PART_SIZE): |
There was a problem hiding this comment.
Independent follow-up review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100e4-4bf8-7bb1-97b6-fb513b7f26a5. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head 6901304bea3cb5112903407b4b7d5d78eb50ed38, new head da55411fa141d3618e7721e3ceed7e580e6c7a67. Static review only. Covered: _upload_chunk/close/commit/discard incl. abort failures and deferred commits; both executors' shutdown idempotency and waiting; fsspec 2026.9.0 write/flush/close/__del__; sync/async pipe_file with bytes/bytearray/memoryview and both dispatch paths; the four named tests. No defects in the executor-shutdown repair. Result: FINDINGS (1).
P2 — Strided multi-byte memoryviews now fail and can overwrite the destination with an empty object (introduced by the incremental diff). With the default block size, memoryview(b"a" * (12 * 2**20)).cast("I")[::2] holds 6 MiB in 1,572,864 items; it used to reach bytes(value) (PutObject), but the byte-based dispatch sends it to the buffered path, whose BytesIO.write raises BufferError; the context manager then closes an empty buffer and commit() touches the object.
Author verification: confirmed (io.BytesIO().write(memoryview(b"a" * 24).cast("I")[::2]) raises BufferError).
There was a problem hiding this comment.
Repaired in cf3cddf: the PutObject dispatch uses len(value) again, exactly as on the base, and only the part limit check uses memoryview(value).nbytes. New test_pipe_file_non_contiguous_memoryview (4 items, 8 bytes, block_size=6) asserts PutObject with the converted bytes; on da55411 it takes the buffered path instead. Self-review of the repair: the dispatch is identical to the base, so no new behavior. Pre-existing and out of scope (recorded in the PR body): a non-contiguous view larger than a block in items still fails in the buffered path, and an exception inside fsspec's pipe_file context manager commits an empty object. Offline tests/pyathena/filesystem/: 174 passed, the same 108 credential-less integration failures; just lint passes.
There was a problem hiding this comment.
Independent follow-up review (relayed) of the repair — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100ea-cdb7-7562-8119-c8e1bb323dd4. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head da55411fa141d3618e7721e3ceed7e580e6c7a67, new head cf3cddf712c746ef72f8a9daec94cb851ee367c2. Static review only. Covered: the pipe_file byte-based pre-check and both upload paths for bytes, bytearray, and contiguous/non-contiguous memoryviews; _upload_chunk/close/commit/discard incl. append accounting, abort failure, deferred commit, and cleanup without an explicit close; both executors' shutdown; fsspec 2026.9.0 write/flush/close/__del__; AioS3FileSystem._pipe_file; the named tests and the new regression test. Result: CLEAN — the pre-check stays byte-based while the dispatch restores the base's exact len(value).
S3 multipart uploads accept at most 10,000 parts. S3File numbered write parts without an upper bound, so a write of more than 10,000 blocks (about 48.8 GiB with the default 5 MiB block size) submitted part 10,001 and failed only when S3 rejected it. Multipart copies with a small block_size had the same gap. Writes now raise ValueError before submitting a part beyond the limit, naming the block size to use. The multipart upload is aborted and the file is closed without its buffered data, so that neither close() nor a deferred commit() uploads a partial object. Parts copied from the existing object in an append count toward the limit. Multipart copies know the source size, so _get_copy_ranges raises the split size to the size divided by 10,000 instead. Closes #953 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If AbortMultipartUpload failed, its error replaced the part limit error, the file stayed open so close() retried the abort, and a deferred commit() completed the parts uploaded so far. Close the file before the abort, log an abort failure, clear the upload state, and always raise the part limit error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In an append to an object smaller than the maximum part size, the copy takes one part, so a block size of the total size divided by 10,000 can still need 10,001 parts. The error now asks for a block size that fits the parts, including the copied ones, and the docs note that the copied parts count toward the limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
S3File.close() skipped the executor shutdown when the final flush raised, as the part limit error does, and the file was already marked closed. Shut it down in a finally block. The part limit failure tests asserted the number of executed part uploads, which depends on whether the abort cancels queued uploads. They now record the submitted parts through a wrapped executor and check its shutdown. The docs claimed one part per block for every write, but each write through open() that fills the buffer uploads its data beyond the last full block as a separate part. Narrow the sizing guidance to put and pipe. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A remainder smaller than the minimum part size is merged into the preceding part, so only a remainder of at least 5 MiB adds a part. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
put_file() and pipe_file() know the size of the data, so they now raise ValueError before uploading anything when it needs more than 10,000 blocks, naming the minimum block size, instead of failing after about 48.8 GiB has been uploaded at the default block size. The check follows the documented rule (a block size of at least the size divided by 10,000), so pipe_file() also rejects data up to 5 MiB over 10,000 blocks, whose short last block the multipart upload would merge. put_file() now accepts block_size and passes it to open(); before, it went to the S3 API as a request parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A write() that reached the part limit closed the file without shutting down its executor, and fsspec does not close a closed file again when it is garbage collected, so the executor was left running unless close() was called. Shut it down in the error path as well. pipe_file() measured a memoryview with len(), which counts its items, so a view with multi-byte items passed the part limit check and the PutObject dispatch with a fraction of its size. Use its size in bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measuring the dispatch in bytes sent a non-contiguous memoryview with multi-byte items to the buffered path, whose BytesIO cannot write it, when its item count fit in a block. Only the part limit check needs the size in bytes; the dispatch keeps using len() as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cf3cddf to
a5f0ec7
Compare
| filesystem's `default_block_size`, must be between 5 MiB and 5 GiB, inclusive, the part | ||
| size limits of a multipart upload. Otherwise, `open` raises `ValueError`. | ||
|
|
||
| A multipart upload consists of at most 10,000 parts. `put` and `pipe` upload one part |
There was a problem hiding this comment.
Rebase onto master after #992 — self-review of the rebase (both perspectives). Old: base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head cf3cddf712c746ef72f8a9daec94cb851ee367c2. New: base 6f258501a56124f6e29c6d530ce429b702ed2ab3 (merge of #992), head a5f0ec7ab45619d563865e6616bb333d6640511e. Result: CLEAN.
git range-diff f3ef301d..cf3cddf7 6f258501..a5f0ec7a: commits 2, 3, 5, 7, 8 identical; 1, 4, 6 differ only in context. Conflicts resolved by each commit's own delta: docs/filesystem.md keeps #992's block-size range paragraph followed by this PR's 10,000-part paragraph; the test import combines S3AioExecutor (#992) and S3ThreadPoolExecutor (this PR).
Upstream contracts that affect this PR, checked: #992's discard() now waits for running parts before AbortMultipartUpload, so the part-limit error path (close, drop buffer, discard(), shut down the executor) aborts after in-flight parts settle; this also settles the in-flight-parts deferral recorded in round two. #992's S3File.__init__ limits the write block size to 5 MiB–5 GiB; 10,000 × 5 GiB exceeds the 5 TiB object limit, so the messages asking for a larger block_size stay satisfiable for any storable object.
Validation on the rebased head: just lint, just docs lint pass; offline tests/pyathena/filesystem/: 219 passed, and the 110 failures are the same set as on 6f258501 (190 passed there; AWS integration tests stopped without credentials).
There was a problem hiding this comment.
Independent review of the rebase (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100f7-b3b6-70d3-b0c4-d61bb06fb921. Old f3ef301d..cf3cddf7, new 6f258501a56124f6e29c6d530ce429b702ed2ab3..a5f0ec7ab45619d563865e6616bb333d6640511e; given the range-diff, the full rebased diff, and the upstream history. Static review only. Covered: the conflict resolutions (both docs paragraphs; test imports keep wait, chain, pairwise, and both executors); the lifecycle through __init__ version-ID handling, _upload_chunk, discard (waits for running uploads before the abort), close, and deferred commit; put_file block-size forwarding, the byte-based pipe_file check with the preserved PutObject dispatch, copy-range sizing, messages and docstrings; the tests against the new initializer validation and wait(). Result: CLEAN — no actionable defects or rebase regressions.
WHAT
Keep S3 multipart uploads within the 10,000-part limit.
S3File): before submitting a part beyondS3FileSystem.MULTIPART_UPLOAD_MAX_PARTS(10,000),_upload_chunk()aborts the multipart upload and raisesValueError. The error asks for ablock_size(or the filesystem'sdefault_block_size) large enough for the object to fit in 10,000 parts. The file is closed and its buffer dropped before the abort, so neitherclose()nor a latercommit()of a deferred write uploads a partial object. An abort failure is logged and does not replace theValueError. The file's executor is shut down in the error path too, because fsspec does not close an already-closed file again when it is garbage collected. In an append, the parts copied from the existing object count toward the limit. This applies toopen(),pipe_file(), andput_file(), and toAioS3File, which inherits_upload_chunk().put_file(),pipe_file()): before uploading anything, a newS3FileSystem._check_multipart_upload_size()raisesValueErrorwhen the data needs more than 10,000 blocks at the block size in use (size > block_size * 10,000, the documented rule). The message names the minimum block size,max(ceil(size / 10,000), 5 MiB).pipe_file()measures the data in bytes (memoryview(value).nbytes) for this check; its PutObject dispatch keepslen(value)as before. The check runs beforeexists()formode="create", before any API call, and before the transaction branch.AioS3FileSystem._put_file/_pipe_filedelegate to these methods.put_file()block_size:put_file()acceptsblock_sizeand passes it toopen(); before, it went to the S3 API as a request parameter and failed.cp_file(),AioS3FileSystem._cp_file(), append copy):_get_copy_ranges()raises the split size toceil(size / 10,000)whenblock_sizeis smaller. This is at most about 550 MB for an object within the 5 TiB limit.S3File.close(): shuts its executor down in afinallyblock, so a failing final flush, such as the part limit error, no longer skips the shutdown.docs/filesystem.mdstates the limit, the about 48.8 GiB maximum forputandpipeat the default block size, how to set a larger block size, that a file written withopencan take more parts (each write that fills the buffer uploads its data beyond the last full block as a separate part), and that an append's copied parts count toward the limit.The maintainer chose to fail writes with a clear error rather than grow the part size during the write, and to check up front where the size is known (
put,pipe). A streamedopen()write cannot be checked in advance because its total size is unknown (fsspec passes no size for writes). The design of the up-front check was reviewed withclaude-fable-5-1.Release-note items:
ValueErrorand aborts its multipart upload. The error is raised when part 10,001 would be submitted, after the first 10,000 parts were uploaded. Before, part 10,001 was submitted and S3 rejected it (per the API documentation; not reproduced).put()/put_file()andpipe()/pipe_file()raiseValueErrorbefore uploading anything when the data needs more than 10,000 blocks, naming the minimum block size.pipe_file()also rejects data up to 5 MiB over 10,000 blocks, which a multipart upload could have fit by merging its short last block.put()/put_file()acceptblock_size.S3File.close()shuts its executor down even when the final flush fails.block_sizenow uses larger parts instead of failing at 10,000 parts.Pre-existing and out of scope:
put_file()forwardsmax_workersto the S3 API;pipe_file()with a non-contiguous memoryview larger than a block in items fails in the buffered path, where an exception inside fsspec'spipe_filecontext manager commits an empty object.WHY
Closes #953.
S3 accepts part numbers from 1 to 10,000.
S3Filenumbered write parts with no upper bound._get_copy_ranges()(from #955) split copies byblock_sizewithout checking the number of parts.TEST
Tested commit: a5f0ec7 (rebased onto 6f25850, the merge of #992).
just formatandjust lint: pass.--noconftest):TestS3FileSystem::test_get_copy_ranges_max_partsuses the real 10,000-part limit and covers exactly 10,000 parts, one byte over, 50 GiB, and 5 TiB.TestS3File::test_write_max_parts,test_write_exceeding_max_parts, andtest_write_exceeding_max_parts_abort_failureuse a limit of 3 on a mocked filesystem. They cover the limit reached bywrite()and byclose(), an append whose copied part counts toward the limit, an abort failure, andautocommitTrue/False, including acommit()after the error.TestS3FileSystem::test_check_multipart_upload_size,test_put_file_exceeding_max_parts,test_put_file_block_size, andtest_pipe_file_exceeding_max_partscover the boundary, the minimum block size in the message, noopen()/API call on rejection (with an explicitblock_sizeand withdefault_block_size), andblock_sizereachingopen(). All 8 cases fail without the source change of 6901304; a memoryview case andtest_write_exceeding_max_parts_without_closefail without da55411, andtest_pipe_file_non_contiguous_memoryviewguards the unchanged PutObject dispatch.close()fail without theclose()change of b8224b1.tests/pyathena/filesystem/offline: 219 passed. The 110 failures are the same set as on the base commit (190 passed there): AWS integration tests stopped byNoCredentialsError, so no request was sent.🤖 Generated with Claude Code