From b364be27cc5555fe997bb110dcf8fcf9deb89374 Mon Sep 17 00:00:00 2001 From: Stefan Jansen Date: Tue, 8 Sep 2026 17:11:09 -0400 Subject: [PATCH] Name the day and the reason when an async Binance download fails The async daily path returned a bare exception and logged `f"Failed to download: {result}"`. `str(exc)` is empty for an exception raised with no arguments, so the line rendered with no subject and no reason at all. Run 33988501549 of the book repository's reader-install job emitted 569 of them, which is what turned a five-symbol Binance outage into a whole seven-dataset job failing with nothing to read. The premium-index monthly path had the same shape at debug level. `fetch_one` now carries the date out with the failure, and both sites log structured fields through `_failure_reason`, which falls back to the exception's type when it carries no message: Failed to download date=2024-01-02 reason=TimeoutError symbol=BTCUSDT Four tests, including one that drives the async daily path with an argument-less `TimeoutError` and asserts the old subject-less line is not what comes out. `uv run pytest tests/ -q`: 3,623 passed, 280 deselected. ruff and ty clean. --- src/ml4t/data/providers/binance_public.py | 38 +++++++++++--- tests/test_binance_public_edge_cases.py | 60 ++++++++++++++++++++++- 2 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/ml4t/data/providers/binance_public.py b/src/ml4t/data/providers/binance_public.py index 5802f19..bb289ec 100644 --- a/src/ml4t/data/providers/binance_public.py +++ b/src/ml4t/data/providers/binance_public.py @@ -42,6 +42,19 @@ async def _gather_or_cancel[ResultT](awaitables: list[Awaitable[ResultT]]) -> li raise +def _failure_reason(error: BaseException) -> str: + """Describe an exception that may carry no message of its own. + + `str(exc)` is empty for an exception raised with no arguments, and the async + download paths logged nothing but that. Run 33988501549 of the book repository's + reader-install job emitted 569 lines reading `Failed to download:` with no + subject and no reason, which is what turned a five-symbol Binance outage into an + undiagnosable failure of the whole job. + """ + message = str(error).strip() + return f"{type(error).__name__}: {message}" if message else type(error).__name__ + + class BinancePublicProvider(BaseProvider): """Provider for bulk historical data from Binance Public Data repository. @@ -1407,24 +1420,31 @@ async def _fetch_daily_data_async( async def fetch_one( date: datetime, url: str - ) -> tuple[datetime, pl.DataFrame | None] | Exception: + ) -> tuple[datetime, pl.DataFrame | None] | tuple[datetime, Exception]: async with semaphore: try: df = await self._download_and_parse_zip_async(url) return (date, df) except Exception as exc: - return exc + # Carry the date out with the failure; without it the warning + # cannot say which day of the window was missing. + return (date, exc) tasks = [fetch_one(date, url) for date, url in urls] results = await _gather_or_cancel(tasks) # Collect successful results in order all_data: list[pl.DataFrame] = [first_df] - for result in results: + for date, result in results: if isinstance(result, Exception): - logger.warning(f"Failed to download: {result}") + logger.warning( + "Failed to download", + symbol=symbol, + date=date.date().isoformat(), + reason=_failure_reason(result), + ) continue - date, df = result + df = result if df is not None and not df.is_empty(): all_data.append(df) @@ -1787,7 +1807,13 @@ async def fetch_month(year: int, month: int) -> pl.DataFrame | None | Exception: all_data: list[pl.DataFrame] = [] for i, result in enumerate(results): if isinstance(result, Exception): - logger.debug(f"Monthly fetch failed for {months[i]}: {result}") + year, month = months[i] + logger.debug( + "Monthly fetch failed", + symbol=symbol, + month=f"{year}-{month:02d}", + reason=_failure_reason(result), + ) continue if result is not None and not result.is_empty(): all_data.append(result) diff --git a/tests/test_binance_public_edge_cases.py b/tests/test_binance_public_edge_cases.py index f6dc533..ea408df 100644 --- a/tests/test_binance_public_edge_cases.py +++ b/tests/test_binance_public_edge_cases.py @@ -5,6 +5,7 @@ """ import io +import re import zipfile from datetime import UTC, datetime from unittest.mock import MagicMock, patch @@ -14,7 +15,7 @@ import pytest from ml4t.data.core.exceptions import DataValidationError -from ml4t.data.providers.binance_public import BinancePublicProvider +from ml4t.data.providers.binance_public import BinancePublicProvider, _failure_reason class TestMarketValidation: @@ -258,3 +259,60 @@ def test_session_has_follow_redirects(self): def test_default_rate_limit(self): """Test default rate limit is set.""" assert BinancePublicProvider.DEFAULT_RATE_LIMIT == (1000, 60.0) + + +class TestAsyncDownloadFailuresAreLegible: + """An async download failure must say which day failed and why. + + `str(exc)` is empty for an exception raised with no arguments, and the async + daily path logged nothing else. Run 33988501549 of the book repository's + reader-install job emitted 569 lines reading `Failed to download:` - no symbol, + no date, no reason - which is what turned a five-symbol Binance outage into an + undiagnosable failure of a whole seven-dataset job. + """ + + def test_an_argumentless_exception_still_names_its_type(self) -> None: + class Unexplained(Exception): + pass + + assert _failure_reason(Unexplained()) == "Unexplained" + + def test_a_message_is_kept_alongside_the_type(self) -> None: + assert _failure_reason(ValueError("HTTP 404")) == "ValueError: HTTP 404" + + def test_a_whitespace_only_message_does_not_produce_a_dangling_colon(self) -> None: + assert _failure_reason(RuntimeError(" ")) == "RuntimeError" + + def test_the_daily_async_path_logs_the_date_and_the_reason(self, capsys) -> None: + """The regression: a bare exception used to render as `Failed to download:`.""" + import asyncio + + provider = BinancePublicProvider() + + async def explode(url: str): + raise TimeoutError + + async def first_available(**kwargs): + return (datetime(2024, 1, 1, tzinfo=UTC), pl.DataFrame()) + + with ( + patch.object(provider, "_download_and_parse_zip_async", side_effect=explode), + patch.object(provider, "_find_first_available_date_async", side_effect=first_available), + ): + asyncio.run( + provider._fetch_daily_data_async( + "BTCUSDT", + "1h", + datetime(2024, 1, 1, tzinfo=UTC), + datetime(2024, 1, 3, tzinfo=UTC), + ) + ) + + # structlog's console renderer colours the fields, so drop the escapes first. + rendered = re.sub(r"\x1b\[[0-9;]*m", "", capsys.readouterr().out) + assert "Failed to download" in rendered + assert "date=2024-01-02" in rendered + assert "symbol=BTCUSDT" in rendered + assert "reason=TimeoutError" in rendered + # The defect: a subject-less, reason-less line. + assert "Failed to download:" not in rendered