Repository navigation
Open
Labels
Description
Part of #1327 (design problems 2, 3 and 9 of the audit report).
Summary
Remove the duplication between S3FileSystem and AioS3FileSystem, move helpers to the objects they belong to, and remove dead code, so that the two filesystems cannot drift.
Current state (766d79d)
- Four delegation styles in
AioS3FileSystem: thread delegation to the synchronous method, reimplementation over private synchronous helpers orcore, direct synchronous calls on the caller's thread (metadata, tags, xattrs,chmod, multipart listings,touch,rmdir), and fsspec defaults. Its class docstring says the internalS3FileSystemkeeps all boto3 logic in one place; several methods callcoredirectly. - Private reach-ins:
_find,_delete_paths,_delete_batches,_delete_batch,_raise_delete_errors,_move_pairs,_copy_pairs,_split_version_paths,S3File._get_ranges,S3FileSystem._get_lookup_kwargs(fromS3FileTargetandS3File),_S3WriteSessionand_S3WriteState. - Duplicated code:
_open(and the open defaults a third time inopen_async),_pipe_file_in_transactionand_put_file_in_transaction,_copy_file, the version preambles ofcopy,getandexpand_path, and the read and write drivers ofS3FileandAioS3StreamedFile(__init__,_get_request_kwargs,_initiate_upload,_upload_chunk,commit/_complete_upload,_fetch_range). Abort-failure logging is written four times. - Already diverged:
mvstops at the first failed copy in sync and runs every copy in aio (pinned bytest_mv_copy_failure); annotations are copied sequentially in sync and in parallel in aio, with about 50 lines of stop logic; appends (Keep the caller's parameters and the open-time pin in S3File #1334). - Helpers on the wrong object:
_directory_object, the inline root entry ofinfo()(which duplicatesS3Object.from_bucket),_extract_parent_directories,_delete_batchesand_raise_delete_errors(static methods over the delete models), the "key itself or keys underkey/" filtering written three times (list_multipart_uploads,object_version_info,_ls_object_versions),S3Core._multipart_upload_request,S3PathPairing._is_glob,_target(which re-implementsS3Path.target) anddelete_paths, block-size validation in three places. - Dead code:
S3FileSystem._calland the_client/_retry_configaliases (used only as test seams),_extra_tokenize_attributesin both classes (fsspec reads class attributes, so it is alwaysNone), the unreachable branch ofinfo()ats3.py:698-701, the duplicate invalidation input_file, theS3Object.getoverride; the_create_executordocstring describes subclasses that do not exist. - Missing on
AioS3FileSystem: the s3fs attributes thatS3FileSystemhas (default_block_size,max_workers,version_aware,s3_additional_kwargs,allow_bucket_creation, ...) and coroutine forms of the request-sending methods in 1.
Proposal (for decision)
- A file-class
ClassVaronS3FileSystem, so thatAioS3FileSystem._opendelegates and the open defaults live once. - Share the transaction with the internal filesystem, so the
_*_in_transactioncopies go. - One public method for the version pairing of
copy/get/expand_path, used by both filesystems; delegate_expand_path. - Move the helpers in 5 to their objects (
S3Object.from_prefix/root,S3DeleteBatch.from_paths,S3DeleteResult.raise_for,S3Pathmethods), and one abort-or-log method on the write session. - One annotation-copy implementation and one
mvfailure policy for both filesystems. - A public write session shared by
S3FileandAioS3StreamedFile, so that they keep only fsspec buffering. - Remove the dead code; tests use
fs.coreinstead offs._call.
Risks: mostly private API; 5 changes the behavior of one filesystem; the tests that assign fs._call (212 uses in test_s3.py) need rewriting, which overlaps #1328.
Activity