fix: improve LLM API error messages for better debugging - #200
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds targeted error handling and propagation for LLM HTTP/JSON failures, fortifies streaming SSE error emission, introduces ray actor timeout utilities, expands chunking/contextualization controls, adds external-resource error detection, CI API test infra and mock VLLM, and multiple test suites and config additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant LLMComponent
participant ExternalAPI
participant RayActor
Client->>Router: POST /v1/chat.completions (stream)
Router->>LLMComponent: forward request (streaming)
LLMComponent->>ExternalAPI: open streaming HTTP connection
alt ExternalAPI returns status >= 400 or raises HTTPStatusError
ExternalAPI-->>LLMComponent: error response
LLMComponent->>Router: raise/convert to ValueError
Router->>Client: SSE error payload
Router->>Client: SSE DONE
else ExternalAPI streams chunks
ExternalAPI-->>LLMComponent: data chunks (JSON)
LLMComponent->>Router: parsed chunks
Router-->>Client: SSE stream chunks
Router->>Client: SSE DONE
end
Note over Router,RayActor: Separate non-stream flow using Ray tasks
Client->>Router: request triggering Ray actor work
Router->>RayActor: submit task (ObjectRef)
RayActor-->>Router: task in progress / completed
alt task hangs / timeout
Router->>RayActor: call_ray_actor_with_timeout cancels task
Router->>Client: error/status reflecting timeout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
openrag/components/llm.py (3)
39-45: Add exception chaining to preserve traceback context.When re-raising exceptions, use
raise ... from eto maintain the exception chain. This helps with debugging by preserving the original traceback.♻️ Proposed fix
except httpx.HTTPStatusError as e: error_detail = e.response.text raise ValueError( f"LLM API error ({e.response.status_code}): {error_detail}" - ) + ) from e except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in API response: {str(e)}") + raise ValueError(f"Invalid JSON in API response: {e}") from e
71-75: Uselogger.exceptionto capture full stack trace.
logger.errorwithstr(e)loses the stack trace. Uselogger.exceptionfor automatic traceback inclusion, which aids debugging.♻️ Proposed fix
except ValueError: raise except Exception as e: - logger.error(f"Error while streaming chat completion: {str(e)}") + logger.exception("Error while streaming chat completion") raise
87-93: Add exception chaining here as well for consistency.Same issue as in the
completionsmethod—preserve the exception chain for better debugging.♻️ Proposed fix
except httpx.HTTPStatusError as e: error_detail = e.response.text raise ValueError( f"LLM API error ({e.response.status_code}): {error_detail}" - ) + ) from e except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in API response: {str(e)}") + raise ValueError(f"Invalid JSON in API response: {e}") from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openrag/components/llm.py
🧰 Additional context used
🪛 Ruff (0.14.10)
openrag/components/llm.py
41-43: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
41-43: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Use explicit conversion flag
Replace with conversion flag
(RUF010)
66-68: Abstract raise to an inner function
(TRY301)
66-68: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
89-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-91: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (2)
openrag/components/llm.py (2)
63-68: Good approach for streaming error handling.Reading the response body with
await response.aread()before accessingresponse.textis the correct pattern for streaming responses. This properly surfaces the LLM provider's error details as intended by the PR objective.
1-2: Thejson.JSONDecodeErrorexception handling is correct. According to httpx documentation,Response.json()raisesjson.JSONDecodeErrordirectly when encountering invalid JSON, not a wrappedhttpx.DecodingError. The import statement and exception handlers at lines 44-45 and 92-93 are properly implemented.However,
Response.json()can also raiseUnicodeDecodeErrorif the response bytes cannot be decoded to text. This is not currently handled and should be caught alongsideJSONDecodeError.
|
I’ve tested this and it works in the following cases:
It turns out this is a classic streaming response issue. Once a streaming response starts sending data to the client, the endpoint first returns a |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
openrag/components/llm.py (4)
39-45: Preserve exception chain withraise ... from e.The original exception context is lost when re-raising. Use
from eto preserve the traceback for debugging.Suggested fix
except httpx.HTTPStatusError as e: error_detail = e.response.text raise ValueError( f"LLM API error ({e.response.status_code}): {error_detail}" - ) + ) from e except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in API response: {str(e)}") + raise ValueError(f"Invalid JSON in API response: {e}") from e
73-75: Consider usinglogger.exceptionfor automatic traceback.
logger.exceptionautomatically includes the stack trace, which is more useful for debugging thanlogger.errorwith just the message.Suggested fix
except Exception as e: - logger.error(f"Error while streaming chat completion: {str(e)}") + logger.exception("Error while streaming chat completion") raise
87-93: Same issue: preserve exception chain withraise ... from e.For consistency with the suggested fix in the
completionsmethod, apply the same pattern here.Suggested fix
except httpx.HTTPStatusError as e: error_detail = e.response.text raise ValueError( f"LLM API error ({e.response.status_code}): {error_detail}" - ) + ) from e except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in API response: {str(e)}") + raise ValueError(f"Invalid JSON in API response: {e}") from e
71-72: Consider adding a comment explaining the intent.The
except ValueError: raisepattern prevents the subsequentexcept Exceptionfrom swallowing detailed error messages. A brief comment would clarify this intent for future maintainers.- except ValueError: - raise + except ValueError: + # Re-raise to propagate detailed error messages to the caller + raise
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openrag/components/llm.pyopenrag/routers/openai.py
🧰 Additional context used
🧬 Code graph analysis (1)
openrag/routers/openai.py (1)
openrag/models/openai.py (2)
OpenAIChatCompletionRequest(14-30)OpenAICompletionRequest(75-92)
🪛 Ruff (0.14.10)
openrag/routers/openai.py
217-217: Do not catch blind exception: Exception
(BLE001)
221-221: Use explicit conversion flag
Replace with conversion flag
(RUF010)
openrag/components/llm.py
41-43: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
41-43: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Use explicit conversion flag
Replace with conversion flag
(RUF010)
66-68: Abstract raise to an inner function
(TRY301)
66-68: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
89-91: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-91: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (3)
openrag/components/llm.py (1)
2-4: LGTM!The
jsonimport is correctly added to supportjson.JSONDecodeErrorhandling introduced in the error handling blocks.openrag/routers/openai.py (2)
104-106: LGTM!The trailing comma in the type annotation is a minor formatting improvement that helps with future diff readability.
200-228: Good approach for streaming error handling.The pattern correctly addresses the streaming limitation noted in the PR comments: once a
StreamingResponsewith status 200 is sent, HTTP errors can't be propagated. Yielding an error chunk in OpenAI's error format followed by[DONE]is a sensible workaround.The broad
except Exceptionat line 217 is intentional here as a catch-all for streaming errors that cannot be communicated via HTTP status.
736f17e to
e36e196
Compare
Extract error details from LLM API responses instead of showing generic "Invalid JSON in API response" for all failures. Now displays the actual error response from the LLM provider. Fixes #197 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
445c82f to
d6f67ae
Compare
2 participants
Extract error details from LLM API responses instead of showing generic "Invalid JSON in API response" for all failures. Now displays the actual error response from the LLM provider.
Fixes #197
ℹ️ An additional integration test could be added after #194 is merged
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.