Skip to content

Conversation

@ks93
Copy link
Contributor

@ks93 ks93 commented Sep 4, 2025

Adds a timeout parameter to the agents.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.

@cursor
Copy link

cursor bot commented Sep 4, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@codecov
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.95%. Comparing base (787926e) to head (05aec6e).
⚠️ Report is 1 commits behind head on master.

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     
Files with missing lines Coverage Δ
cognite/client/_api/agents/agents.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ks93 ks93 marked this pull request as ready for review September 5, 2025 14:28
@ks93 ks93 requested review from a team as code owners September 5, 2025 14:28
@ks93 ks93 marked this pull request as draft September 5, 2025 14:28
@ks93 ks93 changed the title Update agent execution timeout documentation docs(agents): Update agent execution timeout documentation Sep 5, 2025
@ks93 ks93 marked this pull request as ready for review September 5, 2025 16:46
@ks93 ks93 enabled auto-merge September 5, 2025 16:46
@haakonvt
Copy link
Contributor

haakonvt commented Sep 6, 2025

I’m okay with introducing a higher timeout setting for these agent APIs that we expect to be «long running»

@ks93
Copy link
Contributor Author

ks93 commented Sep 19, 2025

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?

@ks93 ks93 changed the title docs(agents): Update agent execution timeout documentation docs(agents): allow setting timeout in agent/chat endpoint Oct 16, 2025
@ks93 ks93 changed the title docs(agents): allow setting timeout in agent/chat endpoint Add endpoint-specific timeout parameter to agents.chat Oct 16, 2025
@ks93 ks93 changed the title Add endpoint-specific timeout parameter to agents.chat feat(agents): add timeout parameter to chat method Oct 16, 2025
@ks93 ks93 requested a review from haakonvt October 16, 2025 00:25
cursoragent and others added 3 commits October 15, 2025 17:34
Co-authored-by: kelvin.sundli <kelvin.sundli@cognite.com>
Co-authored-by: kelvin.sundli <kelvin.sundli@cognite.com>
@ks93 ks93 force-pushed the cursor/update-agent-execution-timeout-documentation-2afe branch from 0733b52 to b9e6bf7 Compare October 16, 2025 00:36
@ks93 ks93 added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Oct 16, 2025
Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

LGTM

@thorkildcognite
Copy link
Contributor

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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

  1. The style guide specifies that constants should be named in UPPER_SNAKE_CASE. Using a constant for the default timeout avoids magic numbers, improving readability and making it easier to update the value in the future. (link)

@erlendvollset erlendvollset self-assigned this Oct 17, 2025
@erlendvollset erlendvollset added the risk-review-ongoing Risk review is in progress label Oct 17, 2025
@erlendvollset
Copy link
Collaborator

erlendvollset commented Oct 17, 2025

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:

  • ties up connection resources in the service for a long time
  • ties up concurency/rate-limiting resources in the service for a long time
  • makes horizontal scaling harder, since you need to wait for long-running ongoing requests to finish
  • services on the request path will usually set lower timeouts (e.g. emissary and nginx have 60 or 90 sec by default), so you need to ensure your whole stack allows increased timeouts

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.

@erlendvollset erlendvollset added waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Oct 17, 2025
@thorkildcognite
Copy link
Contributor

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?

@ks93
Copy link
Contributor Author

ks93 commented Oct 19, 2025

@miladh @andeplane

See comments from Erlend and Thorkild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants