feat(agents): add timeout parameter to chat method#2316
Conversation
|
Cursor Agent can help with this pull request. Just |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2316 +/- ##
==========================================
- Coverage 90.96% 90.95% -0.02%
==========================================
Files 192 192
Lines 26133 26133
==========================================
- Hits 23773 23768 -5
- Misses 2360 2365 +5
🚀 New features to boost your workflow:
|
|
I’m okay with introducing a higher timeout setting for these agent APIs that we expect to be «long running» |
That would be great @haakonvt . How would we do that? |
Co-authored-by: kelvin.sundli <kelvin.sundli@cognite.com>
Co-authored-by: kelvin.sundli <kelvin.sundli@cognite.com>
0733b52 to
b9e6bf7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a timeout parameter to the agents.chat() method, which is a good addition for handling potentially long-running agent executions. The implementation correctly passes the timeout to the underlying request method, and the changes are well-tested. I've added a couple of suggestions to improve maintainability by replacing the magic number for the default timeout with a constant, in line with the repository's style guide.
| messages: Message | ActionResult | Sequence[Message | ActionResult], | ||
| cursor: str | None = None, | ||
| actions: Sequence[Action] | None = None, | ||
| timeout: int = 60, |
There was a problem hiding this comment.
To improve maintainability and avoid using a magic number, the default timeout value 60 should be defined as a constant. This aligns with the style guide's convention for constants.1
You can define it at the module or class level, for example:
_DEFAULT_AGENT_CHAT_TIMEOUT = 60
And then use it in the method signature:
timeout: int = _DEFAULT_AGENT_CHAT_TIMEOUT
Style Guide References
Footnotes
|
I don't mind letting the user increase the timeout for this API if this is how we want the API to behave, but I would still like to express some concerns around the API design. Having long-running HTTP requests introduces some problems:
Have you considered building an async API for this use case? i.e. one endpoint to start the task and another to poll for results. |
|
How long would the user usually want to bump this? Remembering that I believe there is a general 90 second timeout in the API-gateway already? |
|
See comments from Erlend and Thorkild. |
Adds a
timeoutparameter to theagents.chat()method to handle long-running agent executions. The method now uses_do_request()with a default timeout of 60 seconds, allowing endpoint-specific timeout configuration instead of relying solely on global client timeout settings.