added timeout for llm request #1594
Conversation
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
tests/testing_data/system.yaml (1)
208-208: Document timeout behaviorConsider adding a comment in the YAML file to document the timeout behavior, including what happens when requests exceed the timeout and any retry mechanisms.
url: ${LLM_SERVER_URL:http://localhost} + # Timeout in seconds for LLM API requests. Requests exceeding this duration will raise a timeout exception. request_timeout: ${LLM_REQUEST_TIMEOUT:30}system.yaml (1)
212-212: LGTM! Consider documenting the timeout unit.The addition of the
request_timeoutparameter with a default of 30 seconds is a good improvement for LLM request handling. Consider adding a comment to explicitly document that the timeout value is in seconds.url: ${LLM_SERVER_URL:http://localhost} - request_timeout: ${LLM_REQUEST_TIMEOUT:30} + # Timeout in seconds for LLM requests + request_timeout: ${LLM_REQUEST_TIMEOUT:30}kairon/shared/llm/processor.py (2)
187-192: LGTM! Consider enhancing error logging for timeoutsThe timeout implementation looks good. The default of 30 seconds is reasonable, and retrieving the value from environment configuration provides good flexibility.
Consider adding specific logging for timeout exceptions to help with debugging and monitoring. Example:
http_response, status_code, elapsed_time, _ = await ActionUtility.execute_request_async(http_url=f"{Utility.environment['llm']['url']}/{urllib.parse.quote(self.bot)}/completion/{self.llm_type}", request_method="POST", request_body=body, timeout=timeout) +try: + http_response, status_code, elapsed_time, _ = await ActionUtility.execute_request_async( + http_url=f"{Utility.environment['llm']['url']}/{urllib.parse.quote(self.bot)}/completion/{self.llm_type}", + request_method="POST", + request_body=body, + timeout=timeout) +except asyncio.TimeoutError: + logging.error(f"LLM request timed out after {timeout}s for bot: {self.bot}") + raise
188-188: Consider using a constant for the default timeout valueMove the default timeout value to a constant at the module level for better maintainability.
+DEFAULT_LLM_REQUEST_TIMEOUT = 30 - timeout = Utility.environment['llm'].get('request_timeout', 30) + timeout = Utility.environment['llm'].get('request_timeout', DEFAULT_LLM_REQUEST_TIMEOUT)kairon/shared/actions/utils.py (3)
59-59: Enhance parameter documentation.Consider adding the parameter type and default value to the documentation:
- @param timeout request timeout + @param timeout: Request timeout in seconds (default: None)
Line range hint
73-73: Add specific error handling for timeout exceptions.The current implementation doesn't specifically handle timeout exceptions, which could make debugging timeout issues difficult.
Consider adding specific error handling:
try: response = await client.request(request_method, http_url, request_body, headers, **kwargs) http_response = await response.json() response_headers = response.headers + except asyncio.TimeoutError as e: + logging.error(f"Request timed out after {timeout} seconds: {str(e)}") + raise ActionFailure(f"Request timed out after {timeout} seconds") except (ContentTypeError, ValueError) as e: logging.error(str(e)) if response: http_response = await response.text()
The
execute_request_asynccalls are missing thetimeoutparameter.Ensure all calls to
execute_request_asyncinclude and correctly handle thetimeoutparameter.
kairon/shared/llm/processor.pykairon/shared/live_agent/live_agent.pykairon/actions/definitions/schedule.pykairon/actions/definitions/http.py🔗 Analysis chain
Line range hint
49-73: Verify timeout parameter usage across the codebase.Let's ensure all callers of
execute_request_asynchandle the new timeout parameter appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to execute_request_async to verify timeout parameter usage # Search for execute_request_async calls rg "execute_request_async\(" -A 2Length of output: 2566
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
kairon/shared/actions/utils.py(2 hunks)kairon/shared/llm/processor.py(1 hunks)system.yaml(1 hunks)tests/testing_data/system.yaml(1 hunks)
🔇 Additional comments (2)
tests/testing_data/system.yaml (1)
208-208: LGTM! Verify timeout sufficiency across LLM providers
The 30-second default timeout is reasonable for most LLM requests. However, please verify that this duration is sufficient across all supported LLM providers, especially during high-load scenarios.
kairon/shared/actions/utils.py (1)
49-50: LGTM! Method signature change is backward compatible.
The addition of the optional timeout parameter maintains backward compatibility and follows Python conventions.
| if not timeout: | ||
| timeout = Utility.environment['action'].get('request_timeout', 1) |
There was a problem hiding this comment.
Review default timeout value and add validation.
Issues identified:
- The default timeout of 1 second might be too short for many HTTP requests, potentially causing premature timeouts
- Missing validation for negative timeout values
Consider applying these changes:
if not timeout:
- timeout = Utility.environment['action'].get('request_timeout', 1)
+ timeout = Utility.environment['action'].get('request_timeout', 30)
+ if timeout is not None and timeout <= 0:
+ raise ValueError("Timeout must be a positive number")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not timeout: | |
| timeout = Utility.environment['action'].get('request_timeout', 1) | |
| if not timeout: | |
| timeout = Utility.environment['action'].get('request_timeout', 30) | |
| if timeout is not None and timeout <= 0: | |
| raise ValueError("Timeout must be a positive number") |
Summary by CodeRabbit
New Features
timeoutparameter for HTTP requests, enhancing flexibility in request handling.request_timeoutof 30 seconds for LLM requests.Bug Fixes
Documentation