-
Notifications
You must be signed in to change notification settings - Fork 36
feat(agents): add timeout parameter to chat method #2316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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
haakonvt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/gemini review |
There was a problem hiding this comment.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.