Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/filesystem.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ fs = S3FileSystem(connect(s3_staging_dir="s3://YOUR_S3_BUCKET/path/to/",
# Or with direct credentials (s3fs-compatible arguments).
fs = S3FileSystem(key="YOUR_ACCESS_KEY", secret="YOUR_SECRET_KEY")

# Or with a named profile.
fs = S3FileSystem(profile="YOUR_PROFILE")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round two (factual claims, caller compatibility, AWS operational effects, documentation)

Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 9cc6d013516cb94011ac159d99eddee851f8ea24. Result: CLEAN.

Claims checked:

  • "s3fs names the argument profile": s3fs S3FileSystem keeps unknown constructor kwargs in self.kwargs and builds aiobotocore.session.AioSession(**self.kwargs) (fsspec/s3fs s3fs/core.py, set_session()), and its own docstring example is S3FileSystem(profile="<profile name>"). boto3 Session names it profile_name (Connection._SESSION_PASSING_ARGS, connection.py:85-92). Holds.
  • "AioS3FileSystem and fsspec.filesystem("s3", profile=...) pick up the profile": checked with temporary profile files; both sign with OTHERKEY.
  • "profile_name wins when both are given": covered by the parametrized test case.
  • "An unknown profile raises ProfileNotFound at construction, as profile_name= did": checked for profile=; both go through the same Session(profile_name=...) call.
  • Docstring "used as profile_name when profile_name is not given": an explicit profile_name=None counts as given and keeps the default chain; acceptable and matches the wording.

Existing callers: users who passed profile= and relied on it being ignored now sign with that profile (or get ProfileNotFound). This is the bug fix itself; listed in the PR body as a behavior change for the release notes. profile_name= callers and connection= callers are unchanged.

AWS operator: no new requests. Credential resolution for the profile (including assume-role or SSO profiles) is the same as for profile_name=; botocore defers role assumption until the first request.

Documentation: the new example sits with the other s3fs-compatible constructor arguments in docs/filesystem.md; docs/usage.md:819 covers connection-level profile_name and stays correct. No other docs list the s3fs arguments.

Evidence: offline test locally (5 passed; the profile case fails with the source change reverted). Live AWS tests not run locally; CI runs once Ready.


# Or anonymously for public buckets.
fs = S3FileSystem(anon=True)
```
Expand Down
7 changes: 5 additions & 2 deletions pyathena/filesystem/s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,11 @@ def _get_client_compatible_with_s3fs(self, **kwargs) -> BaseClient:

Accepts the constructor arguments that s3fs users pass through fsspec
storage options — ``key``/``username``, ``secret``/``password``,
``token``, ``anon``, ``use_ssl``, ``endpoint_url``,
``token``, ``profile``, ``anon``, ``use_ssl``, ``endpoint_url``,
``connect_timeout``/``read_timeout``, and the ``client_kwargs`` /
``config_kwargs`` dictionaries — in addition to boto3 session
arguments such as ``region_name`` and ``profile_name``.
arguments such as ``region_name`` and ``profile_name``. ``profile``
is used as ``profile_name`` when ``profile_name`` is not given.

Args:
**kwargs: The filesystem constructor arguments.
Expand Down Expand Up @@ -247,6 +248,8 @@ def _get_client_compatible_with_s3fs(self, **kwargs) -> BaseClient:
}
kwargs.update(creds)
client_kwargs.update(creds)
if profile := kwargs.pop("profile", None):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round one (implementation behavior, public contracts, simplicity, regression coverage)

Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 9cc6d013516cb94011ac159d99eddee851f8ea24. Result: CLEAN.

Covered inventory: pyathena/filesystem/s3.py (_get_client_compatible_with_s3fs() and its callers), docs/filesystem.md, tests/pyathena/filesystem/test_s3.py.

  • Behavior: profile is popped from the local kwargs copy only. S3FileSystem.__init__() still passes the original kwargs to AbstractFileSystem, so profile stays in storage_options and the instance-cache token still separates filesystems by profile, as before. profile_name wins when both are given (setdefault).
  • Callers: the only caller is S3FileSystem.__init__() without a connection (s3.py:180). AioS3FileSystem.__init__() forwards **kwargs to the internal S3FileSystem (s3_async.py:113-123), and fsspec.filesystem("s3", ...) resolves to S3FileSystem once registered; both checked manually to sign with the other profile. With a connection, all s3fs arguments are ignored, as documented in the constructor docstring; unchanged.
  • Failure path: an unknown profile now raises botocore ProfileNotFound at construction (checked), the same as profile_name=. anon=True with profile still builds the session with the profile and signs nothing; s3fs also builds its session from the profile.
  • Test quality: the parametrized test builds clients from temporary config/credentials files with env credentials removed, and fails for {"profile": "other"} with the source change reverted (signs with DEFAULTKEY). Limitation: it reads the resolved access key through botocore's private _request_signer._credentials; botocore has no public accessor on a client.

kwargs.setdefault("profile_name", profile)

session = Session(
**{k: v for k, v in kwargs.items() if k in Connection._SESSION_PASSING_ARGS}
Expand Down
33 changes: 33 additions & 0 deletions tests/pyathena/filesystem/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,39 @@ def test_get_client_compatible_with_s3fs(self):
)
assert fs._client.meta.endpoint_url == "http://localhost:9000"

@pytest.mark.parametrize(
("kwargs", "expected"),
[
({}, "DEFAULTKEY"),
# s3fs names the boto3 profile_name argument "profile".
({"profile": "other"}, "OTHERKEY"),
({"profile_name": "other"}, "OTHERKEY"),
({"profile": "other", "profile_name": "default"}, "DEFAULTKEY"),
],
)
def test_get_client_compatible_with_s3fs_profile(self, monkeypatch, tmp_path, kwargs, expected):
# Only constructs a boto3 client from local profile files; no AWS access.
config = tmp_path / "config"
config.write_text("[default]\n[profile other]\n")
credentials = tmp_path / "credentials"
credentials.write_text(
"[default]\naws_access_key_id = DEFAULTKEY\naws_secret_access_key = secret\n"
"[other]\naws_access_key_id = OTHERKEY\naws_secret_access_key = secret\n"
)
for name in (
"AWS_PROFILE",
"AWS_DEFAULT_PROFILE",
"AWS_ACCESS_KEY_ID",
"AWS_SECRET_ACCESS_KEY",
"AWS_SESSION_TOKEN",
):
monkeypatch.delenv(name, raising=False)
monkeypatch.setenv("AWS_CONFIG_FILE", str(config))
monkeypatch.setenv("AWS_SHARED_CREDENTIALS_FILE", str(credentials))

fs = S3FileSystem(region_name="us-east-1", skip_instance_cache=True, **kwargs)
assert fs._client._request_signer._credentials.access_key == expected

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (relayed reviewer result)

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra (different model from the Claude author), codex exec -s read-only --ephemeral, session 01a100c5-77f5-7ca2-9921-624835c35050.
Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 9cc6d013516cb94011ac159d99eddee851f8ea24; reviewed from a detached snapshot of the head with the literal diff, without the PR number, description, commit message or self-review records. Snapshot and PR worktree verified unchanged afterwards.
Static review only; the reviewer did not run tests.

Result: CLEAN.

Reviewer output:

Covered:

  • Profile alias normalization and explicit profile_name precedence.
  • fsspec storage-option forwarding, instance caching, connection-based callers, and AioS3FileSystem.
  • Regression assertions, temporary profile files, AWS environment cleanup, and CI/session setup.
  • Docstring and documentation accuracy.

CLEAN — No actionable defects found. Distinct credential keys make the test detect an ignored alias; environment cleanup and skip_instance_cache=True prevent ordinary CI credentials or cached instances from masking it.

Static source review only. No edits, builds, tests, or GitHub access.


def test_ls_from_cache_with_cached_object(self):
fs = self._make_fs()
obj = S3Object(
Expand Down
Loading