Add opt-in scale-test tier - #104
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues
- Rudy writer can emit scientific-notation coefficients that the Rudy parser rejects, so a file written by this PR can fail to read back.
Nonblocking issues
- None.
Questions
- None.
Tests run and outcomes
git diff --check origin/main...HEAD: passed.- Targeted Rudy write/read with coefficients
1.0e-5and2.0e10: failed as expected withSyntax Error: Invalid input: '0 0 1.0e-5'. julia +1.12 --project=. -e 'using Pkg; Pkg.test()': passed, 1122 tests.env QUBOTOOLS_SCALE_TESTS=true QUBOTOOLS_SCALE_MAX_N=100000 julia +1.12 --project=. -e 'using Pkg; Pkg.test(; test_args = ["scale-only"])': passed, 176 tests.gh pr checks 104: all reported checks passed.
Merge-readiness
I would not merge this until the blocking issue above is addressed. Because I am reviewing my own PR, I am submitting this as COMMENT; a formal approval/change-request verdict must come from another maintainer.
| println(io, "# Diagonal terms") | ||
|
|
||
| for (i, h) in QUBOTools.linear_terms(Φ) | ||
| println(io, "$(i - 1) $(i - 1) $(α * h)") |
There was a problem hiding this comment.
Blocking: the new writer emits default Float64 strings, so values such as 1.0e-5 or 2.0e10 are valid output here but fail on immediate re-read because the Rudy parser only accepts fixed decimal literals. I reproduced this with a temporary spin model written through Format{:rudy}; read_model fails on 0 0 1.0e-5 with Syntax Error. Please either teach the Rudy parser the same scientific-notation numeric grammar accepted by QUBO, including constant comments, and add an exponent round-trip test, or format Rudy output with a representation the existing parser accepts.
There was a problem hiding this comment.
Addressed in b34b8f5. The Rudy parser now accepts exponent-form numbers for coefficient lines and constant-term comments, and test/unit/library/formats.jl adds a write/read round-trip with exponent-form linear, quadratic, and offset values.
|
Addressed review comments for Commits pushed:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Senior-maintainer review of PR #104 (reviewing my own PR, so this is submitted as COMMENT; a formal approve/request-changes verdict must come from another maintainer).
Setup: fetched origin/main and the head commit b34b8f5; merge-base is 8a4caac. git diff --stat against the merge-base reports 585 insertions, 12 deletions across 10 files, matching the PR's reported totals. Reviewed the head in a clean tree.
The prior blocking issue I raised (Rudy writer emitting scientific-notation coefficients the parser rejected) is now fixed by commit b34b8f5 and verified below. I found no new blocking issues.
Blocking issues
- None.
Nonblocking issues
- Duplicate env-gating helper.
test/runtests.jldefines_scale_tests_requested()andtest/scale/scale.jldefines_scale_tests_enabled(); both parseQUBOTOOLS_SCALE_TESTSagainst the same truthy set. Two copies of the same logic can drift. Consider defining it once (e.g. inscale.jl, included before use) and calling it from both entry points. - Test hardcodes a misspelled warning string.
test/scale/scale.jlmatchesr"Depraction Warning", mirroring the typo insrc/library/synthesis/abstract.jl:6. The test passes only because the source is also misspelled; if the source typo is ever corrected, this scale test breaks silently. Prefer matching a stable substring (e.g.r"please refer to .QUBOLib"), and ideally fix the source typo in a follow-up.
Questions
- Time-based budget assertions.
_scale_timedasserts wall-clockresult.time <= seconds._scale_warmuppre-compilesvalue/form/round-trips but notgenerate, so the SK/Wishart cases (budget 10s) include first-call compilation; locally they took ~1.4-2.6s, but a cold/shared CI runner could approach the ceiling. Since this is a scheduled (non-PR-gating) job the impact is low, but is the intent to rely primarily on the allocation budgets and keep the time ceilings only as coarse regression guards? If so, a one-line comment to that effect would help future maintainers. Closes #100: the issue's acceptance criteria include "scheduled scale-test workflow green at n=100k sparse." I verified locally through n=20000 (the QUBO text round-trip path that previously failed now passes); the 100k case and the scheduled workflow can only be confirmed once the cron job runs. The deliverables (opt-in tier, scheduled workflow, Scalability docs, Rudy read+write) are present, and the related blocker #101 is resolved, soCloses #100looks appropriate; flagging only that the n=100k-green criterion is unverified at review time.
Tests run and outcomes (Julia 1.12.6, clean checkout, Pkg.instantiate() first)
julia +1.12 --project=. -e 'using Pkg; Pkg.test()': passed, 1122 tests. No scale tests ran by default, confirming the tier is fully opt-in.env QUBOTOOLS_SCALE_TESTS=true QUBOTOOLS_SCALE_MAX_N=20000 julia +1.12 --project=. -e 'using Pkg; Pkg.test(; test_args=["scale-only"])': passed, 136 tests (the full 100k run reports 176; I capped n to keep runtime bounded while still exercising the QUBO round-trip at n=20000).- Direct Rudy scientific-notation write/read round-trip (coefficients
1.0e-5,-2.5e10, offset4.25e8): now round-trips exactly andvaluematches; this previously failed and was the prior blocking issue. - Note: I used Julia 1.12 because the committed
Manifest.tomlwas resolved with 1.12 and does not instantiate under the local 1.10; matches the PR's stated test environment.
Merge-readiness
No blocking issues remain. The change is fully opt-in, the default suite is unchanged, and the previously blocking Rudy round-trip is fixed and verified. The nonblocking items above are cleanup, not merge gates. Because this is my own PR I am submitting as COMMENT; a formal approval must come from another maintainer.
| const __TEST_PATH__ = @__DIR__ | ||
| const __SCALE_ONLY__ = "scale-only" in ARGS | ||
|
|
||
| function _scale_tests_requested() |
There was a problem hiding this comment.
Duplicate of _scale_tests_enabled() in test/scale/scale.jl (line 16); both parse QUBOTOOLS_SCALE_TESTS against the same truthy set. Consider defining it once and calling it from both entry points so the two cannot drift.
There was a problem hiding this comment.
Addressed in 3088bf8. test/runtests.jl now always loads test/scale/scale.jl and uses the single _scale_tests_enabled() helper, so the main and standalone scale entry points cannot drift.
|
|
||
| for (label, problem, seconds, mib) in generators | ||
| model = _scale_timed(label, seconds, mib) do | ||
| @test_logs (:warn, r"Depraction Warning") QUBOTools.generate( |
There was a problem hiding this comment.
This matches the misspelled warning "Depraction Warning" from src/library/synthesis/abstract.jl:6. The test passes only because the source is also misspelled; if that typo is corrected the test breaks silently. Prefer a stable substring such as r"please refer to .QUBOLib", and fix the source typo in a follow-up.
There was a problem hiding this comment.
Addressed in 3088bf8. The scale test now matches the stable QUBOLib guidance substring instead of the misspelled warning prefix; the source typo remains a follow-up.
| result = @timed f() | ||
|
|
||
| @info label time = result.time mib = result.bytes / 2.0^20 | ||
| @test result.time <= seconds |
There was a problem hiding this comment.
Wall-clock budget assertion. _scale_warmup does not pre-compile generate, so the SK/Wishart cases include first-call compilation against a 10s ceiling (locally ~1.4-2.6s). On a cold/shared CI runner this could flake. Low impact since this is a scheduled job, but consider documenting that the allocation budgets are the primary guard and time ceilings are coarse-only.
There was a problem hiding this comment.
Addressed in 3088bf8. _scale_timed now documents that time ceilings are coarse regression guards and allocation ceilings are the primary signal.
|
Addressed the latest review comments for Commits pushed:
Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
Performance Report -
|
| case | main |
dev |
diff |
|---|---|---|---|
/constructors/n=128/Model/MOI/bool |
284.470 μs (309.191 μs) ± 438.018 μs | 282.296 μs (306.623 μs) ± 378.184 μs | invariant (-0.76%) |
/constructors/n=128/Model |
709.496 μs (747.963 μs) ± 994.713 μs | 704.369 μs (748.254 μs) ± 1.075 ms | invariant (-0.72%) |
/constructors/n=2048/Model/MOI/bool |
9.098 ms (10.112 ms) ± 21.858 ms | 9.326 ms (10.273 ms) ± 22.178 ms | invariant (2.50%) |
/constructors/n=2048/Model |
29.439 ms (33.185 ms) ± 25.901 ms | 29.708 ms (33.656 ms) ± 27.793 ms | invariant (0.91%) |
/constructors/n=384/Model/MOI/bool |
856.313 μs (932.336 μs) ± 484.767 μs | 851.636 μs (919.506 μs) ± 440.920 μs | invariant (-0.55%) |
/constructors/n=384/Model |
2.511 ms (2.791 ms) ± 1.698 ms | 2.549 ms (2.750 ms) ± 1.789 ms | invariant (1.50%) |
/constructors/tsp/cities=36/Model/MOI/bool |
38.039 ms (39.750 ms) ± 64.866 ms | 38.252 ms (40.907 ms) ± 58.170 ms | invariant (0.56%) |
/constructors/tsp/cities=36/Model |
66.637 ms (69.805 ms) ± 59.187 ms | 67.387 ms (71.397 ms) ± 59.103 ms | invariant (1.13%) |
/conversions/n=128/form/Dict |
202.489 μs (243.905 μs) ± 3.224 ms | 154.979 μs (248.717 μs) ± 3.101 ms | improvement (-23.46%) |
/conversions/n=128/form/Matrix/Float32 |
40.650 μs (48.351 μs) ± 154.768 μs | 40.280 μs (48.161 μs) ± 201.158 μs | invariant (-0.91%) |
/conversions/n=128/form/Matrix |
52.477 μs (62.958 μs) ± 3.106 ms | 56.514 μs (67.560 μs) ± 3.204 ms | invariant (7.69%) |
/conversions/n=128/form/SparseMatrixCSC |
178.403 μs (190.501 μs) ± 212.597 μs | 110.794 μs (142.125 μs) ± 419.042 μs | improvement (-37.90%) |
/conversions/n=128/ising/Matrix |
57.354 μs (69.503 μs) ± 3.338 ms | 55.872 μs (68.972 μs) ± 3.201 ms | invariant (-2.58%) |
/conversions/n=128/qubo/Matrix/min |
655.136 μs (692.070 μs) ± 8.138 ms | 619.714 μs (705.771 μs) ± 7.972 ms | invariant (-5.41%) |
/conversions/n=128/qubo/Matrix |
350.247 μs (379.645 μs) ± 6.097 ms | 335.595 μs (380.962 μs) ± 5.801 ms | invariant (-4.18%) |
/conversions/n=384/form/Dict |
311.479 μs (666.573 μs) ± 5.690 ms | 332.520 μs (425.498 μs) ± 4.850 ms | invariant (6.76%) |
/conversions/n=384/form/Matrix/Float32 |
193.175 μs (244.451 μs) ± 12.593 ms | 189.610 μs (258.391 μs) ± 11.998 ms | invariant (-1.85%) |
/conversions/n=384/form/Matrix |
300.102 μs (393.921 μs) ± 20.400 ms | 299.682 μs (431.491 μs) ± 19.255 ms | invariant (-0.14%) |
/conversions/n=384/form/SparseMatrixCSC |
1.266 ms (1.391 ms) ± 9.530 ms | 330.848 μs (419.609 μs) ± 8.962 ms | improvement (-73.87%) |
/conversions/n=384/ising/Matrix |
304.830 μs (395.644 μs) ± 20.434 ms | 278.221 μs (426.709 μs) ± 18.606 ms | invariant (-8.73%) |
/conversions/n=384/qubo/Matrix/min |
6.166 ms (6.308 ms) ± 42.508 ms | 6.286 ms (6.435 ms) ± 35.926 ms | invariant (1.94%) |
/conversions/n=384/qubo/Matrix |
3.173 ms (3.273 ms) ± 30.585 ms | 3.178 ms (3.368 ms) ± 30.335 ms | invariant (0.14%) |
/evaluation/n=128/value/dense-form |
363.145 μs (396.855 μs) ± 695.221 μs | 364.177 μs (395.694 μs) ± 564.924 μs | invariant (0.28%) |
/evaluation/n=128/value/dict-form |
267.574 μs (268.927 μs) ± 9.562 μs | 267.885 μs (269.107 μs) ± 6.734 μs | invariant (0.12%) |
/evaluation/n=128/value/model |
52.017 μs (82.662 μs) ± 679.506 μs | 51.286 μs (54.300 μs) ± 374.522 μs | invariant (-1.41%) |
/evaluation/n=128/value/sparse-form |
51.175 μs (82.622 μs) ± 675.685 μs | 51.016 μs (66.383 μs) ± 502.904 μs | invariant (-0.31%) |
/evaluation/n=384/value/dense-form |
4.189 ms (4.234 ms) ± 176.255 μs | 4.186 ms (4.205 ms) ± 180.119 μs | invariant (-0.07%) |
/evaluation/n=384/value/dict-form |
500.458 μs (508.550 μs) ± 24.307 μs | 505.085 μs (513.347 μs) ± 14.663 μs | invariant (0.92%) |
/evaluation/n=384/value/model |
173.436 μs (184.131 μs) ± 149.113 μs | 177.211 μs (187.101 μs) ± 188.861 μs | invariant (2.18%) |
/evaluation/n=384/value/sparse-form |
175.308 μs (184.492 μs) ± 159.145 μs | 177.041 μs (187.632 μs) ± 217.250 μs | invariant (0.99%) |
1 participant
Summary
QUBOTOOLS_SCALE_TESTS, withscale-onlysupport and a scheduled GitHub Actions workflow.SampleSethandling.n x nmatrix.Tests
julia +1.12 --project=. -e 'using Pkg; Pkg.test()'passed: 1122 tests.env QUBOTOOLS_SCALE_TESTS=true QUBOTOOLS_SCALE_MAX_N=100000 julia +1.12 --project=. -e 'using Pkg; Pkg.test(; test_args = ["scale-only"])'passed: 176 tests.env QUBOTOOLS_DOCS_SKIP_PLOTS=true julia --project=docs docs/make.jl --skip-deploypassed.Notes
julia +1.12 --project=docs docs/make.jl --skip-deployfails in this local checkout becausedocs/Manifest.tomlwas resolved with Julia 1.10 and the local depot is missingMbedTLS_jllsource metadata for the Julia 1.12 docs environment. The docs build passes with Julia 1.10 and plot rendering skipped, matching the docs test path.Branch Hygiene
mainfix/issue-100-scale-testsorigin/mainat8a4caac0d825f28aecb633484aaa4ee2a56a6b6aCloses #100