Skip to content

Remove the duplication between S3FileSystem and AioS3FileSystem #1340

Open

Description

@laughingman7743

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)

  1. Four delegation styles in AioS3FileSystem: thread delegation to the synchronous method, reimplementation over private synchronous helpers or core, direct synchronous calls on the caller's thread (metadata, tags, xattrs, chmod, multipart listings, touch, rmdir), and fsspec defaults. Its class docstring says the internal S3FileSystem keeps all boto3 logic in one place; several methods call core directly.
  2. 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 (from S3FileTarget and S3File), _S3WriteSession and _S3WriteState.
  3. Duplicated code: _open (and the open defaults a third time in open_async), _pipe_file_in_transaction and _put_file_in_transaction, _copy_file, the version preambles of copy, get and expand_path, and the read and write drivers of S3File and AioS3StreamedFile (__init__, _get_request_kwargs, _initiate_upload, _upload_chunk, commit/_complete_upload, _fetch_range). Abort-failure logging is written four times.
  4. Already diverged: mv stops at the first failed copy in sync and runs every copy in aio (pinned by test_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).
  5. Helpers on the wrong object: _directory_object, the inline root entry of info() (which duplicates S3Object.from_bucket), _extract_parent_directories, _delete_batches and _raise_delete_errors (static methods over the delete models), the "key itself or keys under key/" filtering written three times (list_multipart_uploads, object_version_info, _ls_object_versions), S3Core._multipart_upload_request, S3PathPairing._is_glob, _target (which re-implements S3Path.target) and delete_paths, block-size validation in three places.
  6. Dead code: S3FileSystem._call and the _client/_retry_config aliases (used only as test seams), _extra_tokenize_attributes in both classes (fsspec reads class attributes, so it is always None), the unreachable branch of info() at s3.py:698-701, the duplicate invalidation in put_file, the S3Object.get override; the _create_executor docstring describes subclasses that do not exist.
  7. Missing on AioS3FileSystem: the s3fs attributes that S3FileSystem has (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)

  1. A file-class ClassVar on S3FileSystem, so that AioS3FileSystem._open delegates and the open defaults live once.
  2. Share the transaction with the internal filesystem, so the _*_in_transaction copies go.
  3. One public method for the version pairing of copy/get/expand_path, used by both filesystems; delegate _expand_path.
  4. Move the helpers in 5 to their objects (S3Object.from_prefix/root, S3DeleteBatch.from_paths, S3DeleteResult.raise_for, S3Path methods), and one abort-or-log method on the write session.
  5. One annotation-copy implementation and one mv failure policy for both filesystems.
  6. A public write session shared by S3File and AioS3StreamedFile, so that they keep only fsspec buffering.
  7. Remove the dead code; tests use fs.core instead of fs._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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions