Skip to content

GH-47348: [Python] Include retry_strategy in S3FileSystem.__reduce__ #47350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wingkitlee0
Copy link

@wingkitlee0 wingkitlee0 commented Aug 16, 2025

Rationale for this change

This change fixes the serialization/deserialization of S3FileSystem. Previously the retry_strategy parameter is missed in the serialization. This affects other distributed library like Ray to incorrectly reconstruct a S3FileSystem with no retry_strategy parameter in other processes.

What changes are included in this PR?

This PR adds retry_strategy in S3FileSystem.__reduce__ and a property method to allow access to retry_strategy (a python object). Since S3FileSystem is a cdef class but AWSDefaultRetryStrategy etc are not, object type is used for simplicity.

Are these changes tested?

Updated the unit tests to test for retry_strategy.
Notably the retry_strategy property can be used to verify the correct reconstruction.

Are there any user-facing changes?

No.

Copy link

⚠️ GitHub issue #47348 has been automatically assigned in GitHub to PR creator.

…uce__ method

S3FileSystem's retry_strategy should survive serialization
and deserialization.

Signed-off-by: Kit Lee <[email protected]>
@wingkitlee0 wingkitlee0 marked this pull request as ready for review August 16, 2025 22:30
@raulcd
Copy link
Member

raulcd commented Aug 18, 2025

@github-actions crossbow submit -g python

Copy link

Revision: 983021c

Submitted crossbow builds: ursacomputing/crossbow @ actions-19f6d98dfd

Task Status
example-python-minimal-build-fedora-conda GitHub Actions
example-python-minimal-build-ubuntu-venv GitHub Actions
test-conda-python-3.10 GitHub Actions
test-conda-python-3.10-hdfs-2.9.2 GitHub Actions
test-conda-python-3.10-hdfs-3.2.1 GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-latest GitHub Actions
test-conda-python-3.11 GitHub Actions
test-conda-python-3.11-dask-latest GitHub Actions
test-conda-python-3.11-dask-upstream_devel GitHub Actions
test-conda-python-3.11-hypothesis GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-1.26 GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-latest GitHub Actions
test-conda-python-3.11-pandas-nightly-numpy-nightly GitHub Actions
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.12 GitHub Actions
test-conda-python-3.12-cpython-debug GitHub Actions
test-conda-python-3.13 GitHub Actions
test-conda-python-3.9 GitHub Actions
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5 GitHub Actions
test-conda-python-emscripten GitHub Actions
test-cuda-python-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-python-3-amd64 GitHub Actions
test-debian-12-python-3-i386 GitHub Actions
test-fedora-42-python-3 GitHub Actions
test-ubuntu-22.04-python-3 GitHub Actions
test-ubuntu-22.04-python-313-freethreading GitHub Actions
test-ubuntu-24.04-python-3 GitHub Actions

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I am unsure about the object but I am not sure what the alternative would look like without spending some time.
@rok @AlenkaF can you take a look?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Aug 19, 2025

fs = S3FileSystem(
retry_strategy=AwsDefaultS3RetryStrategy(max_attempts=5))
assert isinstance(fs, S3FileSystem)
assert pickle_module.loads(pickle_module.dumps(fs)) == fs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but should also assert that it's different from a filesystem configured with another strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants