Conversation
bernalde
left a comment
There was a problem hiding this comment.
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.
|
Addressed all body-only review findings in Changes
Verification
No review comments were intentionally declined, and there are no inline The PR remains a draft with no formal review decision; |
bernalde
left a comment
There was a problem hiding this comment.
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
verifiedand 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.rudyfetched 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/SHAwith atomicmktemp+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 | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
Summary
benchmarks/vrp/as the VRP benchmark root, consistent with theshared standalone
benchmarks/conventionSECQUOIA/vrp-qinnovisionrepository instead of shipping duplicate data in every TenSolver package
source tree
78c1e390, withSHA-256 verification for all eight files and no package-test or documentation
coupling
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 theNeurIPS ScaleOPT 2025 TenSolver paper; that study used QPLib instances.
Validation
julia +release --startup-file=no benchmarks/vrp/download_data.jl <temp-dir>sha256sumvalues matched the downloader manifestjulia +release --project=. --startup-file=no -e 'using Pkg; Pkg.test(; coverage=false)'julia +release --project=docs/ --startup-file=no docs/make.jl localgit diff --checkand final PR diff inspectionThe live PR diff contains only the downloader, README, and data ignore rule; no
.rudyfiles 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
mainhistory. The PR remains a draft until the author is ready torequest review.