Skip to content

VRP data for benchmarks - #110

Closed
iagoleal wants to merge 2 commits into
mainfrom
bench/vrp
Closed

iagoleal wants to merge 2 commits into
mainfrom
bench/vrp

Conversation

@iagoleal

@iagoleal iagoleal commented Jul 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • establish benchmarks/vrp/ as the VRP benchmark root, consistent with the
    shared standalone benchmarks/ convention
  • keep the 3.4 MB test set in its canonical
    SECQUOIA/vrp-qinnovision
    repository instead of shipping duplicate data in every TenSolver package
    source tree
  • add an opt-in Julia downloader pinned to source commit 78c1e390, with
    SHA-256 verification for all eight files and no package-test or documentation
    coupling
  • document the source, MIT redistribution terms, Harwood et al. formulation,
    filename scheme, and Rudy-style Ising coordinate format

Provenance correction

These files are byte-for-byte copies of the QInnovision World Challenge 2025
test set generated with smharwood/vrp-as-qubo. They are not data from the
NeurIPS ScaleOPT 2025 TenSolver paper; that study used QPLib instances.

Validation

  • fresh download of all eight files with
    julia +release --startup-file=no benchmarks/vrp/download_data.jl <temp-dir>
  • idempotent second pass verified all eight existing files
  • independent sha256sum values matched the downloader manifest
  • checksum-mismatch path rejected invalid content
  • julia +release --project=. --startup-file=no -e 'using Pkg; Pkg.test(; coverage=false)'
  • julia +release --project=docs/ --startup-file=no docs/make.jl local
  • git diff --check and final PR diff inspection

The live PR diff contains only the downloader, README, and data ignore rule; no
.rudy files remain.

Merge note

This workflow appends commits and does not rewrite the existing branch history.
Squash-merge this PR so the original data-only commit's large blobs do not
enter main history. The PR remains a draft until the author is ready to
request review.

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review — VRP benchmark data (draft)

Draft, data-only: 8 .rudy files (~3.4 MB total, largest test_pb_794_o.rudy at 2.1 MB) under bench/data/vrp/, with no loader or docs yet. The instances are QUBO reductions of VRP problems — sparse # Diagonal terms / # Off-Diagonal terms coordinate text with a constant objective term — stated to come from the NeurIPS ScaleOPT 2025 paper. CI is 11/11 green, as expected since nothing reads the files yet. Because it's a draft I'm posting COMMENT; the notes below are what I'd want resolved before it leaves draft, and none are correctness blockers — the data itself is well-formed. (Findings are in the body rather than inline because GitHub doesn't render a diff hunk for the multi-MB files, so there are no anchorable lines.) Disclosure: I authored the sibling benchmark PR #112, which is why the directory-naming point matters to me.

Findings

Question — should multi-MB benchmark data live in the package tree at all? <!-- gh-review-pr:finding=in-tree-data --> The eight instances total ~3.4 MB of plain text committed directly to the package tree. Julia has no per-file package-ignore, so a registered version's source tarball ships all of bench/ to everyone who Pkg.adds TenSolver — and it stays in git history permanently even if the files are moved later. That cuts against the benchmark-independence goal you raised on #112 (the package shouldn't carry the benchmarks). Worth settling before these land in history: keep them in-tree, or move to Git LFS / a GitHub release asset fetched by the benchmark harness / a separate data repo? If they do stay in-tree, adding a .gitattributes marking *.rudy as -text linguist-generated keeps diffs and language stats sane.

Nonblocking — align the benchmark root directory with #112. <!-- gh-review-pr:finding=bench-dir --> This PR uses bench/data/vrp/; #112 uses benchmarks/knapsack/. Neither exists on main yet, so whichever merges first sets the convention and the other should follow. Since you've asked to land #110 first, I'll move #112 to whatever root you pick here — just confirm bench/ vs benchmarks/.

Nonblocking — provenance, license, and a format README before merge. <!-- gh-review-pr:finding=provenance --> These are third-party instances from the ScaleOPT 2025 paper, so before leaving draft they need: (a) a citation/source plus confirmation they're redistributable under a license compatible with TenSolver's, and (b) a short bench/data/vrp/README documenting the .rudy QUBO format — the constant objective term, 0-indexed diagonal and off-diagonal coordinate entries, symmetric upper-triangular storage — and decoding the filename scheme (test_pb_<N>_o → N variables? what do the test_ prefix and _o suffix mean?). As is, a future maintainer has no way to regenerate or verify these.

Merge-readiness

Fine to keep iterating as a draft. Before it goes ready I'd want: (1) a decision on in-tree vs. out-of-tree data, (2) the benchmark-root directory reconciled with #112, and (3) provenance + license + a short README. This is your call as maintainer on repo architecture; I'm flagging the tradeoffs, not the correctness. This account can't post a formal APPROVE while the PR is draft; main is not branch-protected, so no approval gate is reported.

@bernalde bernalde changed the title VRP data for benchmarks Set up external VRP benchmark data Jul 23, 2026
@bernalde

Copy link
Copy Markdown
Member

Addressed all body-only review findings in a4138ec.

Changes

  • in-tree-data: removed the 3.4 MB duplicate .rudy files from the final PR
    diff and added an opt-in downloader pinned to the canonical
    SECQUOIA/vrp-qinnovision commit, with SHA-256 verification for all eight
    instances.
  • bench-dir: established benchmarks/vrp/, matching the shared
    benchmarks/ root convention.
  • provenance: documented the exact source, MIT redistribution terms,
    Harwood et al. formulation, filename scheme, and Rudy-style Ising format.
    The PR title and body now correct the earlier ScaleOPT attribution; that
    paper used QPLib rather than these QInnovision instances.

Verification

  • fresh eight-file download, idempotent re-verification, independent
    sha256sum comparison, and checksum-mismatch rejection
  • full package test suite passed with Julia 1.12
  • local Documenter build passed
  • git diff --check passed
  • current-head CI is green: all nine Julia matrix jobs, the documentation
    build, and documenter/deploy succeeded on
    a4138ec9e0f4f7fc38b72b39470c177fdd1f1159

No review comments were intentionally declined, and there are no inline
threads to reply to.

The PR remains a draft with no formal review decision; main is not branch
protected and the live merge state is CLEAN. Because this workflow appends
commits, squash-merge the PR so the original data-only commit's large blobs do
not enter main history.

@iagoleal iagoleal changed the title Set up external VRP benchmark data VRP data for benchmarks Jul 23, 2026
@iagoleal

Copy link
Copy Markdown
Collaborator Author

Better to keep #112 as the canonical benchmarking infrastructure.
I'll start another draft after that one is merged.

We will only really need these files to address #1, not really for benchmarking right now.

@iagoleal iagoleal closed this Jul 23, 2026

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review — VRP benchmark data, round 2 (draft)

Re-review at a4138ec, following up my COMMENT at 1e1fc9d. This revision resolves all three earlier findings, and does so well: the 3.4 MB of committed .rudy files are gone, replaced by a pinned, SHA-256-verified download_data.jl fetcher plus a data-dir .gitignore; the root moved to benchmarks/vrp/ (matching #112, so #112 needs no change); and a thorough README.md now carries provenance, an MIT-compatibility statement, the Harwood et al. 2021 citation, the filename decode, and a correct Ising-format spec. Still a draft, so I'm posting COMMENT; no blocking findings.

Verified (ran the new script end-to-end)

  • Fresh fetch of all 8 instances to a scratch dir: every file downloaded and passed its recorded SHA-256 (3.5 MB total, matching the originals).
  • Idempotent re-run: reports verified and skips re-download.
  • Corruption path: appended a byte to one file → the script errors with a clear Checksum mismatch … and does not silently accept or overwrite it.
  • Independent check: test_pb_10_o.rudy fetched from the pinned raw URL matches the recorded checksum byte-for-byte.
  • Static safety: hardcoded HTTPS raw URL at an immutable commit + SHA-256 verify; no eval/shell/secret access; stdlib Downloads/SHA with atomic mktemp+mv. Clean.

Findings (all non-blocking)

Question — Julia Artifacts vs. a hand-rolled fetcher. See inline on download_data.jl. Short version: this reimplements a subset of what Artifacts.toml + LazyArtifacts give for free; there's a good decoupling reason to prefer the script, worth a README line so it reads as deliberate.

Nonblocking — add benchmarks/Project.toml. <!-- gh-review-pr:finding=benchmarks-project --> The script runs bare because Downloads/SHA are stdlibs, but on #112 you asked for a benchmarks/Project.toml so the benchmarks form their own environment that depends on TenSolver as if external. Adding one now (even minimal) makes that environment explicit and will be needed as soon as the harness pulls in a non-stdlib dep. Coordinate with #112, which also lives under benchmarks/.

Nonblocking — PR body now contradicts the README. <!-- gh-review-pr:finding=stale-body --> The description still says "Add the VRP benchmarks from the NeurIPS ScaleOPT 2025 paper," but the new README explicitly states these are not from that paper (its study used QPLib) and instead come from SECQUOIA/vrp-qinnovision via smharwood/vrp-as-qubo. Please update the body to match the README so the provenance is consistent.

Nonblocking (minor) — corruption recovery. See inline on download_data.jl.

Merge-readiness

Strong revision; the data-handling design is now sound and decoupled from the package. Before it leaves draft I'd want the PR body reconciled with the README and a decision on the Artifacts-vs-script question (a README line suffices if the script is deliberate). Nothing yet consumes the instances — the loader/harness that reads these Ising files into TenSolver is presumably next — and the downloader isn't exercised by CI (the data is gitignored), so any future benchmark test must invoke it first. This account can't APPROVE while the PR is draft; main isn't branch-protected, so no approval gate is reported.

@@ -0,0 +1,80 @@
using Downloads

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question / design: this reimplements a subset of what Julia's Artifacts system (Artifacts.toml + LazyArtifacts: hash-verified lazy download with depot caching and dedup) provides out of the box. Did you consider and reject it here? There's a solid reason to prefer this script — it keeps the benchmark data fully decoupled from the package (no LazyArtifacts dependency, no Artifacts.toml shipped in the package tree), which serves the same package/benchmark independence you raised on #112. If that's the rationale, a one-line note in the README would make the choice read as deliberate rather than as overlooking the stdlib mechanism. Not blocking either way — the script is clean and correct.


function download_instance(data_dir, instance)
destination = joinpath(data_dir, instance.name)
if isfile(destination)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonblocking (minor): when an existing file fails its checksum, this errors rather than re-downloading, so a user with a corrupted or partial file has to delete it by hand to recover. Erroring is a defensible, safe default — it surfaces the problem instead of masking it — I only flag it because self-healing (re-download on mismatch) is the friendlier alternative if you'd prefer it. I verified both branches locally: intact files report verified, and a corrupted one raises the mismatch error.

@iagoleal
iagoleal deleted the bench/vrp branch July 24, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants