feat: add timeouts, retry logic, and improved error handling#81
feat: add timeouts, retry logic, and improved error handling#81whereisfarukk wants to merge 0 commit intovlm-run:mainfrom
Conversation
Summary of ChangesHello @whereisfarukk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the resilience and stability of the VlmRunClient by introducing comprehensive error management. It incorporates dynamic timeouts for API requests, including an extended duration for chat completions, and implements a robust retry strategy with exponential backoff to gracefully handle transient network issues. Furthermore, error processing has been centralized into a new utility method, ensuring consistent and informative error reporting throughout the client. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable improvements by adding timeouts, retry logic, and centralized error handling to the VlmRun client. The changes enhance the client's robustness. My review includes a few suggestions for improvement: correcting the retry backoff strategy to be exponential as described, simplifying a condition in the retry logic for better readability, extracting a duplicated endpoint string into a constant, and restoring important debugging logs that were removed during the error handling refactoring.
nodes/VlmRun/VlmRunClient.ts
Outdated
| (!error.response && attempt < MAX_RETRIES); // Network errors without response | ||
|
|
||
| if (isRetryable && attempt < MAX_RETRIES) { | ||
| const delay = RETRY_DELAY * attempt; // Exponential backoff |
There was a problem hiding this comment.
The current implementation uses linear backoff (RETRY_DELAY * attempt), but the comment and the pull request description state it should be exponential. For a true exponential backoff, you could use something like RETRY_DELAY * Math.pow(2, attempt - 1).
| const delay = RETRY_DELAY * attempt; // Exponential backoff | |
| const delay = RETRY_DELAY * Math.pow(2, attempt - 1); // Exponential backoff |
| } catch (error: any) { | ||
| // Extract error details similar to the original implementation | ||
| let errorDetail = error.message; | ||
| if (error.response?.body) { | ||
| try { | ||
| const errorBody = | ||
| typeof error.response.body === 'string' | ||
| ? JSON.parse(error.response.body) | ||
| : error.response.body; | ||
| errorDetail = errorBody.detail || errorBody.message || error.message; | ||
| } catch { | ||
| errorDetail = error.response.body || error.message; | ||
| } | ||
| } | ||
| // Log full error details for debugging | ||
| console.error('VlmRun API Error:', { | ||
| url, | ||
| method, | ||
| status: error.response?.status, | ||
| statusText: error.response?.statusText, | ||
| body: error.response?.body, | ||
| errorDetail, | ||
| }); | ||
| throw new Error(`HTTP ${error.response?.status || 'Error'}: ${errorDetail}`); | ||
| this.handleRequestError(error); | ||
| } |
There was a problem hiding this comment.
The refactoring to use handleRequestError is a good improvement for centralizing error logic. However, this change has removed the valuable console.error logging that was present in the previous implementation. This logging provided important context for debugging (like URL and method). It would be beneficial to restore this logging. A good approach would be to enhance handleRequestError to accept and log this additional context.
nodes/VlmRun/VlmRunClient.ts
Outdated
|
|
||
| // Determine timeout based on endpoint | ||
| let timeout = DEFAULT_TIMEOUT; | ||
| if (endpoint === '/openai/chat/completions') { |
There was a problem hiding this comment.
The endpoint path '/openai/chat/completions' is hardcoded here and also in the chat.completions method on line 377. To avoid duplication and improve maintainability, it would be better to define this as a constant and reuse it in both places.
nodes/VlmRun/VlmRunClient.ts
Outdated
| const isRetryable = | ||
| errorCode === 'ETIMEDOUT' || | ||
| errorCode === 'ECONNRESET' || | ||
| errorCode === 'EPIPE' || | ||
| (!error.response && attempt < MAX_RETRIES); // Network errors without response |
There was a problem hiding this comment.
The condition attempt < MAX_RETRIES within the isRetryable check is redundant, as this is already checked in the if condition on line 228. This makes the code slightly harder to understand. It's better to keep the isRetryable flag focused only on whether the error type is retryable.
| const isRetryable = | |
| errorCode === 'ETIMEDOUT' || | |
| errorCode === 'ECONNRESET' || | |
| errorCode === 'EPIPE' || | |
| (!error.response && attempt < MAX_RETRIES); // Network errors without response | |
| const isRetryable = | |
| errorCode === 'ETIMEDOUT' || | |
| errorCode === 'ECONNRESET' || | |
| errorCode === 'EPIPE' || | |
| !error.response; // Network errors without response |
1 participant
Timeouts for chat completions (4 minutes)
Retry logic with exponential backoff for network errors
Centralized error handling