Skip to content

Fix caching in CI to properly handle incremental updates #3728

Description

@costajohnt

tests/__init__.py:128-141 calls concurrent.futures.wait(futures) but never iterates the returned futures or calls .result() on them. Any exception raised in a worker thread is silently captured and never surfaced.

The code

def download_test_pdfs() -> None:
    pdfs = read_yaml_to_list_of_dicts(Path(__file__).parent / "example_files.yaml")

    with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor:
        futures = [
            executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"])
            for pdf in pdfs
        ]
        concurrent.futures.wait(futures)

Symptom

When a download fails (flaky network, rate limit, external host hiccup), download_test_pdfs() returns without raising. The test suite then proceeds assuming every file is local. The first failure surfaces several seconds later as a pytest FileNotFoundError when a test tries to open a missing file, which masks the real cause (the actual download failure from minutes earlier).

Observed impact

This surfaced in at least one Windows CI run: the cache restore missed, so download_test_pdfs() was triggered, and one of the fetches from raw.githubusercontent.com failed silently. The test that eventually opened ASurveyofImageClassificationBasedTechniques.pdf failed with a misleading FileNotFoundError: ...tests/pdf_cache/ASurveyofImageClassificationBasedTechniques.pdf. Investigating the root cause required correlating the pytest failure back to the network download step, which is more work than necessary for what is essentially "the host had a hiccup."

Proposed fix

Iterate the futures and call .result() on each to re-raise worker thread exceptions:

with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor:
    futures = [
        executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"])
        for pdf in pdfs
    ]
    for future in concurrent.futures.as_completed(futures):
        future.result()  # re-raises any exception from the worker

If the intent is to tolerate partial failures, collect exceptions and raise a summary:

errors = []
for future in concurrent.futures.as_completed(futures):
    try:
        future.result()
    except Exception as e:
        errors.append(e)
if errors:
    raise RuntimeError(
        f"Failed to download {len(errors)} test PDF(s). First error: {errors[0]}"
    ) from errors[0]

Either way, failures should be visible at the source, not masked as downstream FileNotFoundError.

Context

Spotted while investigating a Windows CI flake on another PR. The symptom was a confusing FileNotFoundError on a test file that looked unrelated to the PR's changes. The actual root cause was this silent swallow plus a flaky external download, neither of which the PR could have caused.

Metadata

Metadata

Assignees

No one assigned

    Labels

    nf-testingNon-functional change: Testing

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions