Skip to content

feat(sagemaker): pass through async InvocationTimeoutSeconds#332

Closed
ggprior wants to merge 2 commits into
mainfrom
georg/invocation-passthrough
Closed

feat(sagemaker): pass through async InvocationTimeoutSeconds#332
ggprior wants to merge 2 commits into
mainfrom
georg/invocation-passthrough

Conversation

@ggprior

@ggprior ggprior commented Jul 7, 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.

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.

@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 an optional invocation_timeout_s parameter to the SageMaker estimator, allowing users to configure the server-side timeout (InvocationTimeoutSeconds) for asynchronous endpoint invocations, and adds unit tests to verify this behavior. The review feedback recommends validating invocation_timeout_s in the constructor to ensure it is within the valid range of 1 to 3600 seconds and does not exceed async_timeout_s. Additionally, it suggests adding unit tests to cover these validation scenarios.

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.

# request before it is failed. Left as the service default when None;
# raise it for long-running requests. Distinct from `async_timeout_s`,
# which only bounds how long the client polls for the result.
self.invocation_timeout_s = invocation_timeout_s

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.

medium

It is highly recommended to validate invocation_timeout_s in the constructor. SageMaker's InvocationTimeoutSeconds must be an integer between 1 and 3600.

Additionally, if invocation_timeout_s is greater than async_timeout_s, the client-side polling will time out before the server-side invocation can complete, which is a configuration mismatch.

Validating this early prevents uploading orphaned input payloads to S3 (which happens in _invoke_async before the SageMaker call is made).

Suggested change
self.invocation_timeout_s = invocation_timeout_s
self.invocation_timeout_s = invocation_timeout_s
if invocation_timeout_s is not None:
if not (1 <= invocation_timeout_s <= 3600):
raise ValueError("invocation_timeout_s must be between 1 and 3600 seconds.")
if async_timeout_s < invocation_timeout_s:
raise ValueError(
f"async_timeout_s ({async_timeout_s}) must be greater than or equal to "
f"invocation_timeout_s ({invocation_timeout_s}) to prevent client-side timeout "
f"before the server-side invocation completes."
)

Comment on lines +52 to +54
def test_timeout_omitted_by_default(self):
kwargs = self._invoke_async_kwargs(None)
self.assertNotIn("InvocationTimeoutSeconds", kwargs)

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.

medium

It would be beneficial to add unit tests to verify that ValueError is raised when invocation_timeout_s is out of bounds (e.g., < 1 or > 3600) or when it is greater than async_timeout_s.

    def test_timeout_omitted_by_default(self):
        kwargs = self._invoke_async_kwargs(None)
        self.assertNotIn("InvocationTimeoutSeconds", kwargs)

    def test_invalid_timeout_raises_value_error(self):
        with self.assertRaises(ValueError):
            TabPFNClassifier(
                endpoint_name="ep",
                use_async=True,
                s3_bucket="bucket",
                invocation_timeout_s=0,
            )
        with self.assertRaises(ValueError):
            TabPFNClassifier(
                endpoint_name="ep",
                use_async=True,
                s3_bucket="bucket",
                invocation_timeout_s=3601,
            )

    def test_timeout_mismatch_raises_value_error(self):
        with self.assertRaises(ValueError):
            TabPFNClassifier(
                endpoint_name="ep",
                use_async=True,
                s3_bucket="bucket",
                async_timeout_s=100,
                invocation_timeout_s=200,
            )

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.
@ggprior ggprior closed this Jul 8, 2026
@ggprior
ggprior deleted the georg/invocation-passthrough branch July 8, 2026 08:01
@ggprior

ggprior commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #333 — same commits, branch renamed to georg/eng-841-invocation-passthrough to link ENG-841. (GitHub's branch rename closed this PR instead of retargeting it.)

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.

1 participant