Better HTTP Error Handling #199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ludomitch
requested review from
sidnarayanan,
jamesbraza,
mskarlin,
maykcaldas and
nadolskit
jamesbraza
approved these changes
Dec 30, 2024
ldp/alg/rollout.py
Outdated
| @@ -42,8 +42,21 @@ def reraise_exc_as(reraise: type[CaughtError], enabled: bool) -> Iterator[None]: | |||
| yield | |||
| except Exception as e: | |||
| if enabled: | |||
| logger.exception(f"Caught {reraise.exc_type} exception.") | |||
| raise reraise(e) from e | |||
| # Add more detailed error information | |||
There was a problem hiding this comment.
I like it, and I like how you set it up with duck typing to support multiple possible HTTP error types. The one tough part of duck typed code is making changes are a bit harder, so it would be good to unit test this.
Do you mind
- Moving this to a free function in
ldp/utils.py - Making a brief unit test of it, with the particular error type we support
Also, feel free to ignore this comment
There was a problem hiding this comment.
Valid comments, thanks – updated
![codeflash-ai[bot]](https://avatars.githubusercontent.com/in/413177?s=60&v=4){kind=small}
Comment on lines
+99
to
+105
| error_details = f"{error!s}" | ||
|
|
||
| if hasattr(error, "response"): | ||
| error_details += f"\nStatus code: {error.response.status_code}" | ||
| try: | ||
| response_data = error.response.json() | ||
| if "detail" in response_data: |
There was a problem hiding this comment.
Suggested change
| error_details = f"{error!s}" | |
| if hasattr(error, "response"): | |
| error_details += f"\nStatus code: {error.response.status_code}" | |
| try: | |
| response_data = error.response.json() | |
| if "detail" in response_data: | |
| error_details = [f"{error!s}"] | |
| error_details.append(f"\nStatus code: {error.response.status_code}") | |
| error_details.extend(( | |
| "\nServer Traceback:\n ", | |
| "\n ".join(response_data["detail"].split("\n")), | |
| "\n", | |
| )) | |
| error_details.append(f"\nResponse body: {error.response.text}") | |
| return "".join(error_details) |
⚡️ Codeflash found optimizations for this PR📄 38% (0.38x) speedup for
|
| Test | Status |
|---|---|
| ⚙️ Existing Unit Tests | ✅ 3 Passed |
| 🌀 Generated Regression Tests | ✅ 21 Passed |
| ⏪ Replay Tests | 🔘 None Found |
| 🔎 Concolic Coverage Tests | ✅ 1 Passed |
| 📊 Tests Coverage | undefined |
⚙️ Existing Unit Tests Details
- codeflash_concolic_jtjebwfs/tmps1heefup/test_concolic_coverage.py
- test_utils.py🌀 Generated Regression Tests Details
import pytest # used for our unit tests
from ldp.utils import format_error_details
# Mocking HTTP responses for testing
class MockResponse:
def __init__(self, status_code=200, json_data=None, text=""):
self.status_code = status_code
self._json_data = json_data
self.text = text
def json(self):
if self._json_data is not None:
return self._json_data
raise ValueError("No JSON data")
from ldp.utils import format_error_details
# unit tests
def test_basic_exception_handling():
# Standard Exception without Response Attribute
error = ValueError("A simple error message")
codeflash_output = format_error_details(error)
# Exception with Custom Message
error = Exception("Custom error message with special characters: !@#$%^&*()")
codeflash_output = format_error_details(error)
def test_http_exception_handling():
# HTTP Exception with Response Attribute and Status Code
error = Exception()
error.response = MockResponse(status_code=404)
codeflash_output = format_error_details(error)
# HTTP Exception with Response Attribute, Status Code, and JSON Detail
error = Exception()
error.response = MockResponse(status_code=400, json_data={"detail": "Error details\nMore details"})
expected_output = "Exception\nStatus code: 400\nServer Traceback:\n Error details\n More details\n"
codeflash_output = format_error_details(error)
def test_error_handling_in_json_parsing():
# HTTP Exception with Non-JSON Response Body
error = Exception()
error.response = MockResponse(status_code=502, text="Bad Gateway")
codeflash_output = format_error_details(error)
# HTTP Exception with Malformed JSON Response
error = Exception()
error.response = MockResponse(status_code=500, text='{"detail": "Incomplete JSON')
codeflash_output = format_error_details(error)
def test_edge_cases():
# Exception with Empty Message
error = ValueError("")
codeflash_output = format_error_details(error)
# HTTP Exception with Empty JSON Detail
error = Exception()
error.response = MockResponse(status_code=400, json_data={"detail": ""})
codeflash_output = format_error_details(error)
def test_large_scale_cases():
# HTTP Exception with Large JSON Detail
error = Exception()
error.response = MockResponse(status_code=500, json_data={"detail": "Error details\n" * 1000})
expected_output = "Exception\nStatus code: 500\nServer Traceback:\n" + " Error details\n" * 1000
codeflash_output = format_error_details(error)
def test_rare_unexpected_edge_cases():
# Exception with Non-Standard Attributes
class CustomExceptionWithAttributes(Exception):
def __init__(self, message, code, extra_info):
super().__init__(message)
self.code = code
self.extra_info = extra_info
error = CustomExceptionWithAttributes("Custom error", 123, "Extra information")
codeflash_output = format_error_details(error)
# HTTP Exception with Array in JSON Detail
error = Exception()
error.response = MockResponse(status_code=400, json_data={"detail": ["Error 1", "Error 2"]})
codeflash_output = format_error_details(error)
# Exception Message with Unicode Characters
error = ValueError("Ошибка: неверный ввод")
codeflash_output = format_error_details(error)
# HTTP Exception with Deeply Nested JSON Detail
error = Exception()
error.response = MockResponse(status_code=500, json_data={"detail": {"level1": {"level2": {"level3": {"level4": "Deep error message"}}}}})
codeflash_output = format_error_details(error)
# HTTP Exception with Circular Reference in JSON
circular_data = {}
circular_data["detail"] = circular_data
error = Exception()
error.response = MockResponse(status_code=500, json_data=circular_data)
codeflash_output = format_error_details(error)
# HTTP Exception with Mixed Types in JSON Detail
error = Exception()
error.response = MockResponse(status_code=400, json_data={"detail": {"message": "Error occurred", "code": 123, "info": ["Detail 1", "Detail 2"], "nested": {"key": "value"}}})
codeflash_output = format_error_details(error)
# codeflash_output is used to check that the output of the original code is the same as that of the optimized code.
from unittest.mock import MagicMock
# imports
import pytest # used for our unit tests
from ldp.utils import format_error_details
# unit tests
def test_basic_exception():
# Test standard exception without response attribute
error = Exception("A basic error occurred")
codeflash_output = format_error_details(error)
def test_value_error():
# Test ValueError without response attribute
error = ValueError("Invalid value provided")
codeflash_output = format_error_details(error)
def test_empty_exception_message():
# Test exception with empty message
error = Exception("")
codeflash_output = format_error_details(error)
def test_exception_with_custom_attributes():
# Test exception with additional custom attributes
error = Exception("Custom error with additional attributes")
error.code = 1234
codeflash_output = format_error_details(error)
def test_exception_with_unicode_characters():
# Test exception with unicode characters in message
error = Exception("Error with unicode: üñîçødë")
codeflash_output = format_error_details(error)
def test_exception_with_no_message():
# Test exception with no message
error = Exception()
codeflash_output = format_error_details(error)
# codeflash_output is used to check that the output of the original code is the same as that of the optimized code.📢 Feedback on this optimization?
sidnarayanan
approved these changes
Dec 30, 2024
| raise reraise(e) from e | ||
| error_details = format_error_details(e) | ||
| logger.exception(f"Caught {reraise.exc_type} exception:\n{error_details}") | ||
| raise reraise(e) from None |
There was a problem hiding this comment.
Why from None instead of from e? I might be missing something obvious here
There was a problem hiding this comment.
Error chaining was adding redundant information. Have a look at the traceback with chaining below:
With chaining (from e)
2024-12-30 20:41:29,924 - ldp.alg.rollout - ERROR - Caught env exception.
Traceback (most recent call last):
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/ldp/alg/rollout.py", line 43, in reraise_exc_as
yield
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/ldp/alg/rollout.py", line 230, in _rollout
obs, tools = await env.reset()
^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 102, in reset
return await super().reset()
^^^^^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 46, in reset
response = await self._post(
^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 42, in _post
response.raise_for_status()
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/httpx/_models.py", line 763, in raise_for_status
raise HTTPStatusError(message, request=request, response=self)
httpx.HTTPStatusError: Server error '500 Internal Server Error' for url 'http://localhost:8080/reset'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/500
Traceback (most recent call last):
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/ldp/alg/rollout.py", line 43, in reraise_exc_as
yield
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/ldp/alg/rollout.py", line 230, in _rollout
obs, tools = await env.reset()
^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 102, in reset
return await super().reset()
^^^^^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 46, in reset
response = await self._post(
^^^^^^^^^^^^^^^^^
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/aviary/env_client.py", line 42, in _post
response.raise_for_status()
File "/Users/ludovicomitchener/Desktop/repos/aviary-internal/.venv/lib/python3.12/site-packages/httpx/_models.py", line 763, in raise_for_status
raise HTTPStatusError(message, request=request, response=self)
httpx.HTTPStatusError: Server error '500 Internal Server Error' for url 'http://localhost:8080/reset'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/500
It doesn't include the source of the http error and shows the reraise_exc_as traceback twice.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when we run a rollout and the environment fails, the reason for failure is not included in the traceback. Instead we get the following unhelpful traceback
Proposed changes
I've updated the error handling for http responses to include the reason for env failure at the top of the traceback: