fix: add retry logic and error handling to dataset download scripts

[ramakrishnap-nv]

Summary

• Add retry with exponential backoff (3 attempts) to download() in both dataset scripts
• Add 60s connection timeout to prevent hanging on unreachable hosts
• Clean up partial files on download failure
• Continue downloading remaining datasets when one fails — report all failures at the end with non-zero exit code
• Remove insecure SSL verification bypass for plato.asu.edu (site has valid cert, fixes SonarQube python:S4830 and python:S5527)

Before

A single unreachable host (e.g. old.sztaki.hu) crashes the entire download batch — no other datasets get downloaded.

After

Failed downloads are retried 3 times with backoff, then skipped. All other datasets continue downloading. Failures are summarized at the end.

Test plan

• Run bash datasets/linear_programming/download_pdlp_test_dataset.sh — verify plato.asu.edu downloads succeed and unreachable hosts are retried then skipped
• Run python regression/get_datasets.py <dir> lp — verify retry and summary behavior
• Verify non-zero exit code when downloads fail

[coderabbitai]

📝 Walkthrough

Walkthrough

Added timeout-aware, retrying download functions with exponential backoff and partial-file cleanup; ensured parent directories exist; tightened subprocess calls with check=True; refactored CLI loops to accumulate per-dataset failures and exit with status 1 if any fail.

Changes

Cohort / File(s)	Summary
Benchmark LP download utility benchmarks/linear_programming/utils/get_datasets.py	Added sys and time imports. Changed download(url, dst) → download(url, dst, max_retries=3, timeout=60). Implemented retry loop using urlopen(..., timeout=timeout) with exponential backoff (2**attempt), deletion of partially written dst before retry, and re-raise after final failure. Ensure parent dirs via os.makedirs(..., exist_ok=True). Added check=True to subprocess.run(...) calls. Refactored main() to aggregate datasets, catch per-dataset exceptions, record failures, and sys.exit(1) if any failed.
Regression download utility regression/get_datasets.py	Same signature change and retry/timeout behavior: download(url, dst) → download(url, dst, max_retries=3, timeout=60). Ensure parent dirs with os.makedirs(..., exist_ok=True). Remove partial files on retry, use exponential backoff, add check=True to subprocess.run(...). Change control flow to catch per-dataset exceptions, accumulate failed names, log errors, and exit with sys.exit(1) on any failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly summarizes the main changes: adding retry logic and error handling to dataset download scripts, which aligns with the substantial improvements to robustness and error handling across the modified files.
Description check	✅ Passed	The pull request description is directly related to the changeset, detailing the rationale, changes made, behavioral improvements, and providing a concrete test plan aligned with the modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (3)

benchmarks/linear_programming/utils/get_datasets.py (2)

727-751: Clean refactoring with minor duplication consideration.

The consolidated datasets_to_download list approach is cleaner than separate loops. One observation:

If a dataset appears in multiple sources (e.g., both LPFeasibleMittelmannSet and a benchmark suite), it will be added to datasets_to_download multiple times. The download() function's early if os.path.exists(dst): return handles this gracefully, but you could deduplicate for cleaner output:

💡 Optional: Deduplicate datasets

     datasets_to_download = []
     if args.LPfeasible:
         datasets_to_download.extend(LPFeasibleMittelmannSet)
     if args.datasets:
         datasets_to_download.extend(args.datasets)
     if args.benchmarks:
         for bench in args.benchmarks:
             datasets_to_download.extend(
                 MittelmannInstances["benchmarks"][bench]
             )
+    # Deduplicate while preserving order
+    datasets_to_download = list(dict.fromkeys(datasets_to_download))

     for name in datasets_to_download:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/linear_programming/utils/get_datasets.py` around lines 727 - 751,
The list datasets_to_download can contain duplicates from
LPFeasibleMittelmannSet, args.datasets and MittelmannInstances lookups;
deduplicate it before the download loop to avoid repeated attempts and noisy
output. After you populate datasets_to_download (from LPFeasibleMittelmannSet,
args.datasets and the loop over args.benchmarks/MittelmannInstances), convert it
to an ordered unique sequence (e.g., preserve first-seen order using a seen set
or dict.fromkeys) and then iterate over that deduplicated list when calling
download_dataset(name, instance_download_path) and appending failures to failed.

---

632-655: Consider extracting shared download logic.

This download() function is identical to the one in regression/get_datasets.py. To reduce duplication and ensure consistent behavior, consider extracting it to a shared utility module.

This is a minor refactor that could be deferred if the two scripts are expected to evolve independently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@benchmarks/linear_programming/utils/get_datasets.py` around lines 632 - 655,
The duplicate download() implementation should be extracted into a shared
utility function and imported where needed: create a single helper (e.g.,
download function in a new module like utils/downloads or datasets/utils) that
exposes the same signature (download(url, dst, max_retries=3, timeout=60)), move
the retry/read/write logic there (including os.path.exists checks, mkdir,
exponential backoff, cleanup on failure, and raising the final exception), then
replace the local download definitions in both modules with an import of that
shared download function to remove duplication and keep behavior identical.

regression/get_datasets.py (1)

823-846: Retry implementation looks solid.

The exponential backoff (2, 4, 8 seconds) and partial file cleanup are well-implemented. A few observations:

1. URL scheme audit (S310): urlopen accepts file: and other schemes. Since all URLs here are hardcoded from trusted sources, this is low risk. For additional safety, you could validate the scheme:

💡 Optional: URL scheme validation

 def download(url, dst, max_retries=3, timeout=60):
     if os.path.exists(dst):
         return
+    parsed = urllib.parse.urlparse(url)
+    if parsed.scheme not in ("http", "https"):
+        raise ValueError(f"Unsupported URL scheme: {parsed.scheme}")
     os.makedirs(os.path.dirname(dst), exist_ok=True)

2. Memory usage: response.read() loads the entire file into memory. For very large datasets, consider streaming to disk with shutil.copyfileobj(), though this is acceptable for current dataset sizes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@regression/get_datasets.py` around lines 823 - 846, The download function
currently calls response.read() which loads the entire file into memory; change
it to stream the response to disk by opening the destination file and using
shutil.copyfileobj(response, fp) (keeping the existing cleanup behavior that
removes partial dst on exceptions and the exponential backoff/retry logic), and
optionally add a URL scheme check using urllib.parse.urlparse(url) to assert
allowed schemes like "http" or "https" before calling urllib.request.urlopen to
guard against file: and other schemes; keep the same function name download and
preserve max_retries, timeout, and the retry/wait semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@benchmarks/linear_programming/utils/get_datasets.py`:
- Around line 727-751: The list datasets_to_download can contain duplicates from
LPFeasibleMittelmannSet, args.datasets and MittelmannInstances lookups;
deduplicate it before the download loop to avoid repeated attempts and noisy
output. After you populate datasets_to_download (from LPFeasibleMittelmannSet,
args.datasets and the loop over args.benchmarks/MittelmannInstances), convert it
to an ordered unique sequence (e.g., preserve first-seen order using a seen set
or dict.fromkeys) and then iterate over that deduplicated list when calling
download_dataset(name, instance_download_path) and appending failures to failed.
- Around line 632-655: The duplicate download() implementation should be
extracted into a shared utility function and imported where needed: create a
single helper (e.g., download function in a new module like utils/downloads or
datasets/utils) that exposes the same signature (download(url, dst,
max_retries=3, timeout=60)), move the retry/read/write logic there (including
os.path.exists checks, mkdir, exponential backoff, cleanup on failure, and
raising the final exception), then replace the local download definitions in
both modules with an import of that shared download function to remove
duplication and keep behavior identical.

In `@regression/get_datasets.py`:
- Around line 823-846: The download function currently calls response.read()
which loads the entire file into memory; change it to stream the response to
disk by opening the destination file and using shutil.copyfileobj(response, fp)
(keeping the existing cleanup behavior that removes partial dst on exceptions
and the exponential backoff/retry logic), and optionally add a URL scheme check
using urllib.parse.urlparse(url) to assert allowed schemes like "http" or
"https" before calling urllib.request.urlopen to guard against file: and other
schemes; keep the same function name download and preserve max_retries, timeout,
and the retry/wait semantics.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b882ae10-14be-456c-8676-d95311e9eb53

📥 Commits

Reviewing files that changed from the base of the PR and between cfa7ce4 and 5c92ff5.

📒 Files selected for processing (2)

• benchmarks/linear_programming/utils/get_datasets.py
• regression/get_datasets.py

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@benchmarks/linear_programming/utils/get_datasets.py`:
- Around line 740-751: The failure summary doesn't include extraction failures
because extract() uses subprocess.run(..., shell=True) without check=True so
non-zero exits aren't raised and failures aren't added to the failed list;
update extract() (the function that calls subprocess.run for
bzip2/gunzip/gcc/./emps) to either pass check=True to subprocess.run or
explicitly check CompletedProcess.returncode and raise an exception or return
False on non-zero, and ensure download_dataset() or its caller catches those
extraction failures and appends the dataset name to the failed list (the same
list populated on exceptions from download_dataset()), so any extraction error
is reported and causes sys.exit(1).
- Around line 734-738: The loop that extends datasets_to_download from
MittelmannInstances["benchmarks"][bench] can raise KeyError for typos in
args.benchmarks; update the loop that iterates over args.benchmarks to first
check that bench exists in MittelmannInstances["benchmarks"] (or use
.get("benchmarks", {}) and test membership) and skip invalid names while logging
or warning (but do not raise), so the batch continues; modify the block
referencing args.benchmarks, MittelmannInstances["benchmarks"], and
datasets_to_download accordingly to perform safe lookup and continue on unknown
benchmark names.

In `@regression/get_datasets.py`:
- Around line 919-939: The failure summary misses extraction errors because
extract() calls subprocess.run(..., shell=True) without check=True so failures
don't raise and the outer try in download_lp_dataset/download_mip_dataset won't
append to failed; update extract() to either use subprocess.run(..., check=True,
shell=True) for every critical command or explicitly check returncode and raise
an exception on non-zero exit, so download_lp_dataset and download_mip_dataset
will catch and cause the failed list to be appended; ensure extract()'s contract
is to raise on any extraction/compile error (or return False and have download_*
raise) so the final sys.exit(1) logic correctly reports and exits on extraction
failures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ef369cd-c535-45d5-9951-f5cfa0f93cc8

📥 Commits

Reviewing files that changed from the base of the PR and between 5c92ff5 and a413a8a.

📒 Files selected for processing (2)

• benchmarks/linear_programming/utils/get_datasets.py
• regression/get_datasets.py

[coderabbitai]

🧹 Nitpick comments (1)

regression/get_datasets.py (1)

892-907: Directory creation handled implicitly.

Unlike the other script's download_dataset() which explicitly calls os.makedirs(dir, exist_ok=True), this function relies on download() to create the parent directory. This works because download() calls os.makedirs(os.path.dirname(dst), exist_ok=True) on Line 826, and dst is os.path.join(dir, ...), so dir gets created.

While functional, consider adding explicit os.makedirs(dir, exist_ok=True) here for consistency with benchmarks/linear_programming/utils/get_datasets.py:713 and clearer intent.

Optional: Add explicit directory creation for consistency

 def download_lp_dataset(name, dir):
     if name not in MittelmannInstances["problems"]:
         raise Exception(f"Unknown dataset {name} passed")
     if os.path.exists(dir):
         if os.path.exists(os.path.join(dir, f"{name}.mps")):
             print(
                 f"Dir for dataset {name} exists and contains {name}.mps. Skipping..."
             )
             return
     url, type = MittelmannInstances["problems"][name]
     if url == "":
         print(f"Dataset {name} doesn't have a URL. Skipping...")
         return
+    os.makedirs(dir, exist_ok=True)
     file = os.path.join(dir, os.path.basename(url))
     download(url, file)
     extract(file, dir, type)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@regression/get_datasets.py` around lines 892 - 907, The download_lp_dataset
function relies indirectly on download() to create the target directory which is
implicit and inconsistent with other modules; update download_lp_dataset to
explicitly ensure the target directory exists by calling os.makedirs(dir,
exist_ok=True) near the start of the function (before building file path and
calling download/extract) while preserving the existing checks for known
problems and empty URLs; reference download_lp_dataset and the existing
download() behavior only as context.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@regression/get_datasets.py`:
- Around line 892-907: The download_lp_dataset function relies indirectly on
download() to create the target directory which is implicit and inconsistent
with other modules; update download_lp_dataset to explicitly ensure the target
directory exists by calling os.makedirs(dir, exist_ok=True) near the start of
the function (before building file path and calling download/extract) while
preserving the existing checks for known problems and empty URLs; reference
download_lp_dataset and the existing download() behavior only as context.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a57d6e8a-c834-4d67-b10a-1c5eb5e8d0fc

📥 Commits

Reviewing files that changed from the base of the PR and between a413a8a and 4191018.

📒 Files selected for processing (2)

• benchmarks/linear_programming/utils/get_datasets.py
• regression/get_datasets.py

[Iroy30]

lgtm

[ramakrishnap-nv]

/merge

[ramakrishnap-nv]

/merge