Skip to content

Add timeout option to Python SDK#117

Merged
jakelazaroff merged 1 commit intomainfrom
jake/timeout
Mar 5, 2025
Merged

Add timeout option to Python SDK#117
jakelazaroff merged 1 commit intomainfrom
jake/timeout

Conversation

@jakelazaroff
Copy link
Contributor

@jakelazaroff jakelazaroff commented Feb 24, 2025

Adds timeout to Python SDK via the keyword argument timeout, which accepts either an int or an httpx Timeout object.

The only method missing it is exec — it's a little weird there because we already have timeout_seconds which controls the timeout of the running Python instruction (I've renamed that to instruction_timeout_seconds to better match) and also because we wrap two separate API requests with that one command. Open to suggestions on how to include it there too!

Increases the timeout of exec_result and exec_result_async to 20 minutes, which is longer than users can set the instruction timeout.

@paulgb
Copy link
Member

paulgb commented Feb 24, 2025

Adds timeout to Python SDK via the keyword argument timeout, which accepts either an int or an httpx Timeout object.

The only method missing it is exec — it's a little weird there because we already have timeout_seconds which controls the timeout of the running Python instruction (I've renamed that to instruction_timeout_seconds to better match) and also because we wrap two separate API requests with that one command. Open to suggestions on how to include it there too!

Rather than exposing two timeout parameters, why not just set the HTTP timeout based on the instruction timeout? I think that exposing both timeout and instruction_timeout on the same method is confusing.

@jakelazaroff
Copy link
Contributor Author

Rather than exposing two timeout parameters, why not just set the HTTP timeout based on the instruction timeout? I think that exposing both timeout and instruction_timeout on the same method is confusing.

I agree but I do think they're distinct concerns — like, maybe you know your instruction might take up to 30 seconds to run, but you don't want to potentially wait up to 30 seconds to submit it. But maybe that's too niche to worry about right now and we should just combine them until someone actually wants to set them differently?

@jakelazaroff jakelazaroff merged commit a52a610 into main Mar 5, 2025
2 checks passed
@jakelazaroff jakelazaroff deleted the jake/timeout branch March 5, 2025 14:48
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