Skip to content

feat(sagemaker): pass through async InvocationTimeoutSeconds#333

Merged
ggprior merged 2 commits into
mainfrom
georg/eng-841-invocation-passthrough
Jul 8, 2026
Merged

feat(sagemaker): pass through async InvocationTimeoutSeconds#333
ggprior merged 2 commits into
mainfrom
georg/eng-841-invocation-passthrough

Conversation

@ggprior

@ggprior ggprior commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The SageMaker async estimators previously invoked invoke_endpoint_async with no InvocationTimeoutSeconds, so every async request rode the service default and long-running requests could be failed by the platform before completing with no way to extend the window from the client. This adds an optional invocation_timeout_s constructor argument that, when set, is forwarded as InvocationTimeoutSeconds (the server-side 1–3600s per-request cap); it defaults to None so behaviour is unchanged when unset, and it is deliberately distinct from async_timeout_s, which only bounds how long the client polls S3 for the result. A mocked unit test covers both the pass-through and the default-omitted cases and needs no real boto3.

Part of ENG-841.

ggprior added 2 commits July 7, 2026 15:26
Add an optional invocation_timeout_s to the SageMaker estimators. When set it
is forwarded to invoke_endpoint_async as InvocationTimeoutSeconds, letting
callers raise the server-side per-request cap (1-3600s) for long-running async
requests. Defaults to None (service default), so behaviour is unchanged when
unset. Distinct from async_timeout_s, which only bounds client-side polling.
test_client's authorize() leaves TABPFN_TOKEN on the module-global options,
so test_reload_opts (which asserts it starts as None) fails depending on order.
Add an autouse fixture that snapshots and restores options._opts and the
TABPFN_TOKEN env var around each test. Pre-existing on main; unrelated to the
InvocationTimeoutSeconds change but required for a green suite.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for configuring a server-side invocation timeout (invocation_timeout_s) on SageMaker estimators, passing it to the invoke_endpoint_async call. It also adds a test fixture to isolate global client state between unit tests and includes new unit tests for the timeout parameter. The review feedback suggests validating that invocation_timeout_s is an integer between 1 and 3600, removing a redundant int() cast once validation is in place, and adding unit tests to verify that invalid timeout values correctly raise a ValueError.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/tabpfn_client/sagemaker/estimator.py
Comment thread src/tabpfn_client/sagemaker/estimator.py
Comment thread tests/unit/test_sagemaker_invocation_timeout.py
@ggprior
ggprior marked this pull request as ready for review July 8, 2026 09:41
@ggprior
ggprior requested a review from a team as a code owner July 8, 2026 09:41
@ggprior
ggprior requested review from brendan-priorlabs and simo-prior and removed request for brendan-priorlabs July 8, 2026 09:41
@ggprior
ggprior added this pull request to the merge queue Jul 8, 2026
Merged via the queue into main with commit b474add Jul 8, 2026
16 checks passed
@ggprior
ggprior deleted the georg/eng-841-invocation-passthrough branch July 8, 2026 11:50
This was referenced Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants