Add Julia CI cache#24
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Review as repository maintainer.
I am the PR author, so GitHub does not allow me to submit a formal APPROVE or REQUEST_CHANGES on this PR (HTTP 422); this is submitted as COMMENT. The formal verdict must come from another maintainer. Based on my review there are no blocking issues and the PR is merge-ready.
What the change does: adds julia-actions/cache@v3.1.0 between setup-julia and julia-buildpkg, and bumps actions/upload-artifact from v5 to v7.0.1. CI-only; no package code changes.
Verification:
- Diff confirmed against merge-base e1ad2b4: 1 file, +2/-1, matching the PR's reported totals. The three commits net to exactly these two line changes.
- Both action references resolve to real, current tags:
julia-actions/cache@v3.1.0(a45e8fa, identical to the floatingv3) andactions/upload-artifact@v7.0.1(043fb46, identical to the floatingv7). - The cache step is placed where issue #23 specified (after
setup-julia@v3, beforejulia-buildpkg@v1). - The cache action key is derived from the Julia version and Project/Manifest hashes, so it invalidates correctly; no stale-environment correctness concern.
- CI is green on all four jobs (ubuntu + windows, Julia 1.10 and 1) on run 27117763727.
Intent vs issue #23: the issue is a benchmark-then-decide task. Acceptance criteria are met: no package code changes; cold/warm timings recorded in the PR body and the issue comment; cache restore/save times reported for all four matrix jobs; CI passes on both OSes and both Julia versions; and the warm critical path improves well past the issue's threshold (slowest Windows job 7m51s -> 4m11s, -220s; overall -47%, vs the >=60s or >=20% bar). The issue proposed @v2, but the recorded benchmark was run with @v3.1.0, so the merged version matches what was actually measured. Closes #23 is appropriate (the issue's decision criterion is satisfied and the change is being adopted, not just measured). I take the benchmark numbers as contributor-supplied claims; I did not re-run the timing matrix, but the action tags, placement, and green CI are independently verified.
-
Blocking issues: none.
-
Nonblocking issues:
- Version-pinning inconsistency. The two touched actions are pinned to patch tags (
cache@v3.1.0,upload-artifact@v7.0.1) while every other action in the workflow floats on a major (checkout@v6,setup-julia@v3,julia-buildpkg@v1,julia-runtest@v1,julia-processcoverage@v1). Patch pins won't pick up upstream bugfixes and read inconsistently with the rest of the file. Pick one convention: float to@v3/@v7to match the existing style, or (if pinning for supply-chain reasons) pin all actions to full commit SHAs. Minor.
- Questions:
- The
actions/upload-artifactv5->v7 bump is outside issue #23's stated scope (purely the cache benchmark). It is benign and justified by the PR body (removes the Node 20 deprecation annotation), and CI confirms the artifact still uploads, but it is an unrelated CI-hygiene change folded into a cache PR. Intentional to bundle here, or better as its own commit/PR for a cleaner history? Not blocking either way.
- Tests run and outcomes:
- No package tests are affected by this change. Verified the two action tags exist and resolve to current releases; verified placement against the issue; relied on the PR's CI (run 27117763727), which passes on all four matrix jobs and exercises both the cache step and the updated artifact upload.
- Merge-readiness: merge-ready. Small, CI-only change that meets issue #23's documented merge criterion with green CI on all supported platforms. The nonblocking pinning item and the scope question are optional cleanups.
| with: | ||
| version: ${{ matrix.version }} | ||
| arch: ${{ matrix.arch }} | ||
| - uses: julia-actions/cache@v3.1.0 |
There was a problem hiding this comment.
Nonblocking (minor): this pins to the patch tag v3.1.0, while every other action in this file floats on a major (checkout@v6, setup-julia@v3, julia-buildpkg@v1, etc.). A patch pin won't pick up upstream bugfixes and is inconsistent with the surrounding style. Suggest julia-actions/cache@v3 to match convention, or pin all actions to full SHAs if the intent is supply-chain hardening.
There was a problem hiding this comment.
Addressed in d3d8a33 by switching this to julia-actions/cache@v3. I also verified @v3 currently resolves to the same tag SHA as the benchmarked v3.1.0, and CI run 27119881459 passed with @v3.
| - uses: julia-actions/julia-processcoverage@v1 | ||
| if: matrix.version == '1' && matrix.os == 'ubuntu-latest' | ||
| - uses: actions/upload-artifact@v5 | ||
| - uses: actions/upload-artifact@v7.0.1 |
There was a problem hiding this comment.
Question + same pinning note: bumping upload-artifact v5 -> v7.0.1 is outside issue #23's scope (cache benchmark). It's benign and CI confirms the upload still works, but it's an unrelated change in a cache PR; consider whether it belongs in its own commit/PR. Also @v7.0.1 is a patch pin where the rest of the file floats on majors -- prefer @v7 for consistency.
There was a problem hiding this comment.
Addressed the pinning part in d3d8a33 by switching this to actions/upload-artifact@v7. I kept the artifact bump in this PR rather than splitting it out because it removes the Node 20 warning from the same CI workflow; CI run 27119881459 passed with @v7.
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Blocking issues: none.
Nonblocking issues: none.
Questions: none.
Tests run and outcomes:
- Fetched
origin/mainandorigin/fix/issue-23-cache-benchmark, then diffed from merge-basee1ad2b4883e0f1c2a2c70deb57b93c67a5fe4f13. - Verified GitHub's PR file totals match local diff totals: 1 changed file, +2/-1.
- Read issue #23 and its comments; the PR addresses the requested cache benchmark, records cold/warm timings, confirms passing CI, and uses
Closes #23appropriately. git diff --check: passed.julia --project=. -e 'using Pkg; Pkg.test()': passed, including package metadata, README JuMP workflow, and QUBODrivers tests.gh pr checks 24: passed on all four CI matrix jobs for run 27117763727 attempt 2.
Merge-readiness: merge-ready from my review. Because I am authenticated as the PR author, GitHub does not permit a formal self-approval; this review is submitted as COMMENT, and the formal approval must come from another maintainer.
|
Addressed the unresolved review threads. Commits pushed:
Main changes made:
Tests run and results:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
1 participant
Summary
julia-actions/cache@v3after Julia setup and beforejulia-buildpkg.actions/upload-artifactfromv5tov7, which removes the Node 20 deprecation annotation seen while benchmarking.Benchmark
No-cache medians from #23:
Cold/warm cache benchmark with
julia-actions/cache@v3.1.0on run 27117362896:The cache adds save overhead on cold runs, but warm runs are materially faster. The direct v3 benchmark improved the matrix critical path from 7m51s to 4m11s (-220s, -47%). At review time,
julia-actions/cache@v3resolves to the same tag SHA asv3.1.0.Final branch validation after switching to major action tags used run 27119881459:
The latest final-workflow attempt's matrix critical path was 3m56s, still materially below the 6m37s no-cache baseline critical path from #23. The final workflow passed with
julia-actions/cache@v3andactions/upload-artifact@v7.Validation
git diff --checkactionlint .github/workflows/ci.ymljulia --project=. -e 'using Pkg; Pkg.test()'Closes #23