Repository navigation
Use the connection's s3_config and session credentials in to_sql() - #1068
Conversation
to_sql() built its S3 resources without the connection's botocore config, so proxies, timeouts, retries and the user agent did not apply, and its upload workers built sessions from the connection's keyword arguments only, so the credentials of connect(session=...) were ignored and the uploads used the default credential chain. The resources now get s3_config, and the workers get the frozen credentials of the connection's session, which stay picklable for a ProcessPoolExecutor. Closes #1067 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| session_kwargs = deepcopy(conn._session_kwargs) | ||
| session_kwargs.update({"profile_name": conn.profile_name}) | ||
| credentials = ( | ||
| None if "botocore_session" in session_kwargs else conn.session.get_credentials() |
There was a problem hiding this comment.
Self-review round one (behavior and implementation): FINDINGS (1, repaired)
Base 911492c, head d540ea5 (full diff: pyathena/pandas/util.py, tests/pyathena/pandas/test_util.py, docs/pandas.md).
Covered:
- Every S3 request of
to_sql(): the bucket resource (objects.filter().delete()forif_exists="replace") andto_parquet()'s per-chunkSession(**session_kwargs).resource("s3", **client_kwargs). Both now getconfig=conn.s3_config. - Credentials:
_session_kwargskeeps explicit keys and therole_arn/serial_numbertemporary keys (stored inConnection._kwargs); frozen credentials ofconn.sessionhave the same values in those cases and add theconnect(session=...)case.profile_nameis still passed, so profile settings other than credentials still apply. A session without credentials adds nothing (previous behavior). - Executors: the arguments are a
Configand strings;Configpickles (checked), and the live test passes withProcessPoolExecutor. - Test quality: the test makes the default credential chain invalid after building the connection's session, so it fails on the original source for both executors (
InvalidAccessKeyIdon PutObject) and checksmax_pool_connectionson every resource recorded in the test process.
Finding: with connect(botocore_session=bs), _session_kwargs contains bs, and boto3.Session(botocore_session=bs, aws_access_key_id=...) calls bs.set_credentials(), replacing the user's (possibly refreshable) credentials with static ones (checked locally: the key changed to the new one). Repaired in d764a59: no credentials are added when a botocore session is given; it already carries them. Pre-existing and unchanged: deepcopy(conn._session_kwargs) copies that botocore session, and a botocore session cannot be pickled for a ProcessPoolExecutor.
| ) | ||
| client_kwargs = deepcopy(conn._s3_client_kwargs) | ||
| client_kwargs.update({"region_name": conn.region_name}) | ||
| client_kwargs.update({"region_name": conn.region_name, "config": conn.s3_config}) |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations): CLEAN
Base 911492c, head d764a59 (PR body, commit messages, docstring, docs/pandas.md, issue #1067).
- "Proxies, timeouts, retries,
max_pool_connectionsand the user agent did not apply": master'sconn.session.resource("s3", region_name=..., **conn._s3_client_kwargs)andsession.resource("s3", **client_kwargs)pass noconfig, and_client_kwargsnever containsconfig(it is a namedConnectionparameter). - "The uploads used the default credential chain with
connect(session=...)": reproduced in to_sql() ignores the connection's botocore config and the credentials of connect(session=...) #1067 (workersession_kwargs == {'profile_name': None}), and the live test fails on master with the environment's invalid keys. - "Credentials resolved once":
get_frozen_credentials()is called once perto_sql()call before the chunks are submitted; before, each chunk's new session resolved them again. Stated as a behavior change, with therole_arn/serial_numberprecedent (fixed temporary keys in_kwargs). - Existing callers:
to_sql()andto_parquet()signatures are unchanged;to_parquet()callers that pass their own kwargs are unaffected. - AWS operator: the S3 requests now follow the connection's retry config inside botocore in addition to
retry_api_callaround PutObject, as the result-set requests already do. - Docs: the
docs/pandas.mdsentence links to the existing "S3 client" section;just docs lintpassed.
Freezing the credentials for every connection made refreshable credentials (assume-role profiles, instance and container roles) expire in a long upload, and resolved the session's credentials even when explicit keys take precedence. The workers now resolve the credentials themselves as before, unless the connection was given its session; then they get its credentials as of the start of the uploads. Connection records whether its session was given. The process pool test uses the spawn start method, so that no forkserver keeps the invalid keys of its environment for later pools. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| session_kwargs = deepcopy(conn._session_kwargs) | ||
| session_kwargs.update({"profile_name": conn.profile_name}) | ||
| if ( | ||
| conn._session_given |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS, static review.
- Reviewer: OpenAI Codex CLI 0.160.0, model
gpt-6-astra, effort high, sandboxread-only, session01a10536-d1ce-7181-8df3-a4a8e53134f8. Scope911492c3..d764a591in a detached snapshot without.env; no PR text or earlier findings in the prompt; snapshot unchanged afterwards. - Covered (reviewer's list):
Connectionsession construction, credential precedence,_session_kwargs/_s3_client_kwargs/config merging; replacement listing/deletion, partitioned and unpartitioned uploads, worker serialization, retries; both executors, supplied-session side effects, refreshable/missing credentials, tests, docstring and docs; boto3/botocore 1.43.102 and multiprocessing/pickle sources.
Findings and dispositions:
- (Introduced, P2) Freezing credentials for every connection made the workers' refreshable credentials (assume-role profiles, instance/container roles) expire in a long upload. Verified; redesigned in 4b22102 after the maintainer chose to freeze only for
connect(session=...):Connection._session_givenrecords a given session; otherwise the workers resolve credentials themselves as before.test_to_sql_workers_resolve_credentialschecks that a connection without a session passes no credentials to the workers. - (Introduced, P2)
s3_config=Config(signature_version=UNSIGNED)cannot be pickled forProcessPoolExecutor. Verified; not changed: unsigned requests cannot write a table's data, so this configuration cannot work withto_sql()anyway. Stated in the PR body. - (Introduced, P2) The session's credentials were resolved even when explicit keys take precedence, so a partial-credentials environment could raise
PartialCredentialsError. Repaired in 4b22102: no lookup whenaws_access_key_idis among the client arguments. - (Introduced, P2) Under the
forkserverstart method (Linux default since Python 3.14), the process-pool test could start a forkserver that keeps the test's invalid keys for later pools. Repaired in 4b22102: the test uses aProcessPoolExecutorwith thespawncontext, whose workers exit with the pool. - (Pre-existing) A
botocore_sessionamong the connection arguments is deep-copied for the workers, which fails for one holding refreshable credentials and cannot be pickled for processes. Not changed; the code comment no longer claims the arguments are picklable, and the PR body lists it under "Not changed".
|
|
||
| # Whether the session was given rather than built from the arguments, | ||
| # which then do not reproduce its credentials. | ||
| self._session_given = bool(session) |
There was a problem hiding this comment.
Self-review of the repair (both perspectives), scope d764a5917a96e37c67b1de89c64ab79254cc7a8e..4b221020a1139834d813876791f89d38d4955059: CLEAN
Round one (behavior): _session_given is bool(session), matching the if session: branch that uses it. The freeze condition requires a given session, no explicit client keys, no botocore session, and non-empty credentials; every other connection keeps master's worker arguments, so refresh behavior is unchanged there. test_to_sql_session_credentials_and_s3_config (thread and spawn process pools) fails on master's source and passes; test_to_sql_workers_resolve_credentials passes (it would fail on d764a59, which froze for every connection). tests/pyathena/pandas/test_util.py: 15 passed; just lint passed.
Round two (claims): the docstring and docs/pandas.md now say the workers resolve credentials themselves except for a given session; the PR body's behavior changes limit the once-at-start resolution to connect(session=...). Connection._session_given is private and only read by to_sql(). just docs lint passed.
SQLAlchemy passes aws_access_key_id=None without credentials in the URL, and None does not take precedence over the given session's credentials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| session_kwargs.update({"profile_name": conn.profile_name}) | ||
| if ( | ||
| conn._session_given | ||
| and not conn._s3_client_kwargs.get("aws_access_key_id") |
There was a problem hiding this comment.
Independent follow-up review (relayed): FINDINGS, static review.
- Reviewer: OpenAI Codex CLI 0.160.0, model
gpt-6-astra, effort high, sandboxread-only, session01a10541-28d5-7932-8526-98cd168c702a. Scoped764a591..4b221020(the redesign) in a detached snapshot without.env; snapshot unchanged afterwards. - Covered (reviewer's list): default/profile credentials, explicit keys, assume-role/MFA, supplied boto3/botocore sessions, both executors; cleanup and upload S3 configuration incl. partitions; tests, environment restoration, process isolation, docstrings and docs; boto3/botocore 1.43.102. No test environment or monkeypatch leak found; the spawn context avoids the forkserver problem.
Findings and dispositions:
- (Introduced, P2) The key-presence check skipped forwarding for
aws_access_key_id=None, which botocore treats as unset; SQLAlchemy'screate_connect_args()always passes it (Nonewithout credentials in the URL). Verified and repaired in ab57b06: both checks look at values. New test case[ThreadPoolExecutor-connect_kwargs2](aws_access_key_id=None, aws_secret_access_key=None) fails withInvalidAccessKeyIdon PutObject with the 4b22102 check and passes now. - (Introduced in d764a59, P2)
botocore_session=Nonelikewise suppressed forwarding. Repaired in ab57b06 by the same value check. - (Pre-existing) A botocore session holding refreshable credentials cannot be deep-copied for the workers. Not changed; listed under "Not changed" in the PR body.
- Docs wording omitted the explicit-key precedence. Repaired in ab57b06 (docstring and
docs/pandas.md: "asessionand no explicit keys").
After the repair: just lint, just docs lint, tests/pyathena/pandas/test_util.py: 16 passed.
| if ( | ||
| conn._session_given | ||
| and not conn._s3_client_kwargs.get("aws_access_key_id") | ||
| and not session_kwargs.get("botocore_session") |
There was a problem hiding this comment.
Independent follow-up review of ab57b06 (relayed): CLEAN, static review.
- Reviewer: OpenAI Codex CLI 0.160.0, model
gpt-6-astra, effort high, sandboxread-only, session01a1054a-3c5e-7ce2-b5eb-c951416abdf9. Scope4b221020..ab57b063in a detached snapshot without.env; snapshot unchanged afterwards. - Result: "No actionable defects introduced by this commit. The value checks handle SQLAlchemy's
Nonecredential arguments, and the new test parameter detects the original failure. The wording change introduces no new inaccuracy."
WHAT
pyathena.pandas.util.to_sql()now sends its S3 requests with the connection's botocore settings, and its upload workers use the credentials of a session given toconnect().if_exists="replace") and the upload workers' resources (to_parquet()) getconfig=conn.s3_config, the connection'sconfigmerged withs3_configand PyAthena's user agent (Share one S3 client per connection across result sets and filesystems #1058). Before, they were built without a config, so proxies, timeouts, retries,max_pool_connectionsand the user agent did not apply.conn._session_kwargsandprofile_name, so refreshable credentials (assume-role profiles, instance and container roles) stay refreshable. Only for a connection given its session (connect(session=...), recorded in the new privateConnection._session_given), whose credentials those arguments cannot reproduce, the workers get that session's credentials as of the start of the uploads. Before, such workers used the default credential chain while the bucket resource used the connection's session.botocore_sessionis among the session arguments; both are checked by value, since SQLAlchemy passesaws_access_key_id=Nonewithout credentials in the URL. For a botocore session,boto3.Session(botocore_session=bs, aws_access_key_id=...)would set static credentials on the user's botocore session (checked locally).botocore.config.Configand strings, which pickle forProcessPoolExecutor. A config withsignature_version=UNSIGNEDdoes not pickle; unsigned requests cannot write a table's data anyway.to_sql()anddocs/pandas.mdstate which settings and credentials the S3 requests use.Behavior changes (release-note candidates):
to_sql()'s S3 requests now use the connection'sconfig/s3_config, including its retry settings and proxies.connect(session=...),to_sql()uploads with that session's credentials instead of the default credential chain.connect(session=...), the upload credentials are resolved once, when the uploads start; an upload that outlives temporary credentials of that session fails instead of refreshing. Other connections are unaffected.Not changed: the workers still build their own session and resource per chunk instead of sharing
Connection.s3_client. Abotocore_sessionamong the connection arguments is still deep-copied for the workers, which fails for one holding refreshable credentials and cannot be pickled for processes (pre-existing).WHY
Fixes #1067, a follow-up of #1058.
TEST
Tested commit ab57b06 (base 911492c). AWS CI on ab57b06 (run 37178922657):
test,test-sqlaandtest-sqla-asyncpassed.just lint,just docs lint: passed.uv run --env-file .env pytest -n 2 tests/pyathena/pandas/test_util.py: 16 passed.test_to_sql_session_credentials_and_s3_config, forThreadPoolExecutorand aProcessPoolExecutorwith thespawnstart method (so that no forkserver keeps the test's invalid environment for later pools), and a thread pool case withaws_access_key_id=None, aws_secret_access_key=Noneas SQLAlchemy passes them: the connection gets a session built from the resolved credentials ands3_config=Config(max_pool_connections=37), and the environment then holds invalid keys, so the default credential chain fails. Without the change, the cases fail withInvalidAccessKeyIdon PutObject (theNonecase also fails with the key-presence check of 4b22102); with it, all pass, and everySession.resource()call recorded in the test process (the bucket resource, and with threads the workers' resources) hasmax_pool_connections == 37.test_to_sql_workers_resolve_credentials: for a connection built without a session, the workers' session arguments carry no credentials. It guards the refresh behavior; it passes on master as well.🤖 Generated with Claude Code