diff --git a/py/src/braintrust/logger.py b/py/src/braintrust/logger.py index 550b0d1d..a97c9831 100644 --- a/py/src/braintrust/logger.py +++ b/py/src/braintrust/logger.py @@ -728,16 +728,23 @@ def set_http_adapter(adapter: HTTPAdapter) -> None: _state._api_conn._reset() +#: HTTP status codes that indicate a transient failure worth retrying, rather than a +#: client error that will fail identically on every attempt. +RETRYABLE_STATUS_CODES = frozenset({429, 500, 502, 503, 504}) + + class RetryRequestExceptionsAdapter(HTTPAdapter): """An HTTP adapter that automatically retries requests on connection exceptions. This adapter extends requests' HTTPAdapter to add retry logic for common network-related - exceptions including connection errors, timeouts, and other HTTP errors. It implements - an exponential backoff strategy between retries to avoid overwhelming servers during - intermittent connectivity issues. + exceptions including connection errors, timeouts, and other HTTP errors, as well as + responses that complete with a transient HTTP error status (see `RETRYABLE_STATUS_CODES`). + It implements an exponential backoff strategy between retries to avoid overwhelming + servers during intermittent connectivity issues. Attributes: - base_num_retries: Maximum number of retries before giving up and re-raising the exception. + base_num_retries: Maximum number of retries before giving up and re-raising the exception + (or returning the last error response). backoff_factor: A multiplier used to determine the time to wait between retries. The actual wait time is calculated as: backoff_factor * (2 ** retry_count). default_timeout_secs: Default timeout in seconds for requests that don't specify one. @@ -770,6 +777,21 @@ def send(self, *args, **kwargs): # downloading. if not response.is_redirect and response.content: pass + if response.status_code in RETRYABLE_STATUS_CODES and num_prev_retries < self.base_num_retries: + # Unlike connection-level failures, a completed response with an error + # status doesn't raise -- retry it here so transient 5xx/429 responses + # get the same backoff treatment as network exceptions. + sleep_s = self.backoff_factor * (2**num_prev_retries) + print( + "Retrying request after HTTP", + response.status_code, + "response", + file=sys.stderr, + ) + print("Sleeping for", sleep_s, "seconds", file=sys.stderr) + time.sleep(sleep_s) + num_prev_retries += 1 + continue return response except (urllib3.exceptions.HTTPError, requests.exceptions.RequestException) as e: if num_prev_retries < self.base_num_retries: @@ -4362,6 +4384,7 @@ def summarize( score_summary = {} metric_summary = {} comparison_experiment_name = None + scores_fetch_error = None if summarize_scores: # Get the comparison experiment if comparison_experiment_id is None: @@ -4385,6 +4408,7 @@ def summarize( }, ) except Exception as e: + scores_fetch_error = str(e) _logger.warning( f"Failed to fetch experiment scores and metrics: {e}\n\nView complete results in Braintrust or run experiment.summarize() again." ) @@ -4413,6 +4437,7 @@ def summarize( comparison_experiment_name=comparison_experiment_name, scores=score_summary, metrics=metric_summary, + scores_fetch_error=scores_fetch_error, ) def export(self) -> str: @@ -6002,13 +6027,21 @@ class ExperimentSummary(SerializableDataClass): """Summary of the experiment's scores.""" metrics: dict[str, MetricSummary] """Summary of the experiment's metrics.""" + scores_fetch_error: str | None = None + """If set, fetching the score/metric summary from the server failed with this error, and + `scores`/`metrics` are empty as a result of that failure -- not because the experiment has + no scores. Callers that gate automation (e.g. CI) on `scores` should check this field before + treating an empty `scores` dict as "the experiment has no scores".""" def __str__(self): comparison_line = "" if self.comparison_experiment_name: comparison_line = f"""{self.experiment_name} compared to {self.comparison_experiment_name}:\n""" + fetch_error_line = "" + if self.scores_fetch_error: + fetch_error_line = f"WARNING: failed to fetch scores and metrics ({self.scores_fetch_error}). The summary below does not reflect the experiment's actual scores.\n\n" return ( - f"""\n=========================SUMMARY=========================\n{comparison_line}""" + f"""\n=========================SUMMARY=========================\n{fetch_error_line}{comparison_line}""" + "\n".join([str(score) for score in self.scores.values()]) + ("\n\n" if self.scores else "") + "\n".join([str(metric) for metric in self.metrics.values()]) diff --git a/py/src/braintrust/test_framework.py b/py/src/braintrust/test_framework.py index 5c000bdf..42c42258 100644 --- a/py/src/braintrust/test_framework.py +++ b/py/src/braintrust/test_framework.py @@ -5,6 +5,7 @@ import pytest from braintrust.logger import BraintrustState +from braintrust.util import AugmentedHTTPError from .framework import ( Eval, @@ -164,6 +165,34 @@ def get_json(path, args=None): ) +def test_experiment_summarize_surfaces_scores_fetch_error(with_memory_logger, with_simulate_login): + """A failure to fetch experiment-comparison2 must be distinguishable from a genuine + empty-scores result, not silently collapsed into the same `scores == {}` shape. + + See https://github.com/braintrustdata/braintrust-sdk-python/issues/639. + """ + exp = init_test_exp("test-evaluator", "test-project") + mock_conn = MagicMock() + + def get_json(path, args=None): + if path == "v1/experiment/base-exp-id": + return {"name": "base-exp"} + if path == "experiment-comparison2": + raise AugmentedHTTPError("502 Bad Gateway") + raise AssertionError(f"Unexpected get_json call: {path}, {args}") + + mock_conn.get_json.side_effect = get_json + + with patch.object(exp.state, "api_conn", return_value=mock_conn): + summary = exp.summarize(comparison_experiment_id="base-exp-id") + + assert summary.scores == {} + assert summary.metrics == {} + assert summary.scores_fetch_error is not None + assert "502 Bad Gateway" in summary.scores_fetch_error + assert "WARNING" in str(summary) + + @pytest.mark.asyncio @pytest.mark.skipif(not HAS_PYDANTIC, reason="pydantic not installed") async def test_run_evaluator_exposes_validated_parameter_values_to_hooks(): diff --git a/py/src/braintrust/test_http.py b/py/src/braintrust/test_http.py index b9ede8d8..1432adb8 100644 --- a/py/src/braintrust/test_http.py +++ b/py/src/braintrust/test_http.py @@ -174,6 +174,105 @@ def test_adapter_resets_pool_on_timeout(self, hanging_server): assert HangingConnectionHandler.request_count >= 2 +class TransientErrorStatusHandler(http.server.BaseHTTPRequestHandler): + """HTTP handler that returns a transient error status for the first N requests. + + Simulates a load balancer/proxy hiccup (502/503/504) that completes with a normal + HTTP response rather than raising a connection-level exception -- this does not enter + the exception-handling retry path, only a status-code-aware one. + """ + + request_count = 0 + fail_count = 1 + error_status = 502 + + def log_message(self, format, *args): + pass + + def do_GET(self): + TransientErrorStatusHandler.request_count += 1 + + if TransientErrorStatusHandler.request_count <= TransientErrorStatusHandler.fail_count: + self.send_response(TransientErrorStatusHandler.error_status) + self.send_header("Content-Type", "text/html") + self.end_headers() + self.wfile.write(b"Bad Gateway") + return + + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(b'{"status": "ok"}') + + +@pytest.fixture +def transient_error_server(): + """Fixture that creates a server returning an error status for the first request.""" + TransientErrorStatusHandler.request_count = 0 + TransientErrorStatusHandler.fail_count = 1 + TransientErrorStatusHandler.error_status = 502 + + server = socketserver.ThreadingTCPServer(("127.0.0.1", 0), TransientErrorStatusHandler) + server.daemon_threads = True + port = server.server_address[1] + + thread = threading.Thread(target=server.serve_forever) + thread.daemon = True + thread.start() + + yield f"http://127.0.0.1:{port}" + + server.shutdown() + server.server_close() + + +class TestRetryOnHttpErrorStatus: + """Tests that the adapter retries completed responses with a transient error status. + + Regression coverage for https://github.com/braintrustdata/braintrust-sdk-python/issues/639: + a request that completes with a non-2xx status returns a normal Response rather than + raising, so it must be retried by inspecting the status code, not just by catching + connection-level exceptions. + """ + + def test_adapter_retries_on_502(self, transient_error_server): + adapter = RetryRequestExceptionsAdapter(base_num_retries=3, backoff_factor=0.05) + session = requests.Session() + session.mount("http://", adapter) + + resp = session.get(f"{transient_error_server}/experiment-comparison2") + + assert resp.status_code == 200 + assert resp.json() == {"status": "ok"} + assert TransientErrorStatusHandler.request_count == 2 + + def test_adapter_gives_up_after_base_num_retries(self, transient_error_server): + TransientErrorStatusHandler.fail_count = 100 # always fails + + adapter = RetryRequestExceptionsAdapter(base_num_retries=2, backoff_factor=0.01) + session = requests.Session() + session.mount("http://", adapter) + + resp = session.get(f"{transient_error_server}/experiment-comparison2") + + # 1 initial attempt + 2 retries, then the caller gets the last error response back. + assert resp.status_code == 502 + assert TransientErrorStatusHandler.request_count == 3 + + def test_adapter_does_not_retry_non_retryable_status(self, transient_error_server): + TransientErrorStatusHandler.fail_count = 100 + TransientErrorStatusHandler.error_status = 404 + + adapter = RetryRequestExceptionsAdapter(base_num_retries=5, backoff_factor=0.01) + session = requests.Session() + session.mount("http://", adapter) + + resp = session.get(f"{transient_error_server}/experiment-comparison2") + + assert resp.status_code == 404 + assert TransientErrorStatusHandler.request_count == 1 + + class TestHTTPConnection: """Tests for HTTPConnection timeout configuration."""