ci: share native dependency fingerprint with builder image - #27330
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
loveRhythm1990
left a comment
loveRhythm1990
left a comment
There was a problem hiding this comment.
Reviewed the fingerprint contract together with matrixorigin/CI#426. Overall the design is sound — but note that the fingerprint is the only thing standing between a dependency bump and a stale native build, so its coverage deserves a close look.
Why the fingerprint is the only guard
Every target in thirdparties/Makefile is an existence-only target — e.g.
install/include/xxhash.h:has no prerequisites. So once a consumer restores thirdparties/install, make build will never rebuild a component, even if its tarball changed. There is no second line of defence behind the fingerprint comparison. That makes the coverage table below the whole correctness story.
Coverage
Captured correctly: all top-level tarballs, thirdparties/Makefile (including ONNX_REPO_COMMIT and the per-platform SHA256s), download / download_test, cgo sources, the root Makefile, and go/cc/cxx/cmake versions plus os/arch. A dependency bump changes the fingerprint and forces a source rebuild.
go.mod / go.sum are not captured, which is fine — only thirdparties/install is restored, and the Go module cache is a genuine cache that GOPROXY refills.
1. hash_thirdparties drops thirdparties/cuda/**
find . -maxdepth 1 -type f -print0schema-1 used git rev-parse HEAD:thirdparties, which hashed the whole tree. thirdparties/cuda/Common/ holds several dozen headers that the new -maxdepth 1 no longer sees, so editing them will not invalidate the fingerprint. Today only GPU builds consume those headers and the CPU CI path does not fetch cuda, so the practical risk is low — but this is a capability regression versus schema-1 and it is exactly the class of miss that produces a silently stale native build. Suggest a full-tree walk that excludes generated output instead:
find . -type f -not -path './install/*' -not -path './_*' -print0(Hashing working-tree content rather than the git tree is the right call otherwise — it is strictly more accurate for PR merge trees and dirty checkouts.)
2. xargs -0 sha256sum can hang on empty input
GNU xargs runs the command once with no arguments when input is empty, and sha256sum with no args reads stdin. hash_cgo's suffix filter currently always matches, but if it is ever narrowed this becomes a hang rather than an error. Please add -r / --no-run-if-empty to the three xargs -0 calls.
3. go env GOVERSION and PATH-dependent probes may make the fingerprint never match
Two determinism concerns, both fail-safe (they cause a source rebuild, not a stale restore) but both capable of silently reducing the cache hit rate to zero:
go env GOVERSIONin a directory with atoolchaindirective ingo.modcan trigger toolchain resolution, and reports the toolchain version. The producer runs on main while a consumer runs on a PR branch; if the twogo.modtoolchain lines differ at all, the fingerprint can never match.cmake --versionandcompiler_version ccdepend on PATH. The producer runs this script duringdocker build(with the Dockerfile'sENVapplied); the consumer runs it underdocker run ... bashas a non-login shell. If PATH differs, one side recordsunavailableand the comparison never succeeds.
Worth printing both fingerprints from a real CI run and diffing them before merging, rather than discovering after the fact that the optimisation never engages.
4. schema-1 compatibility branch has no removal trigger
The else branch in Dockerfile.ci-builder and the matching legacy acceptance on the CI side are correct — the old tree hash is actually stricter for thirdparties, and the compiler comes from the image itself so it is consistent by construction. No staleness risk. But nothing marks when it can go away. Please file a follow-up issue to delete both halves once the first schema-2 nightly image is published.
Merge order
Both sides are bidirectionally compatible, so either order is safe. Suggest landing this PR first, letting one nightly produce a schema-2 image, and only then landing matrixorigin/CI#426 — otherwise CI will silently take the fallback path throughout and the concerns in §3 will not surface.
The go env -w GOPROXY="${GOPROXY}" quoting fixes are necessary for the chained http://...|https://goproxy.cn|direct proxy on the CI side; CI#426 patches old MatrixOne checkouts with sed to compensate, which should be cleaned up once this lands.
matrix-meow
added
size/M
|
Addressed the fingerprint review findings in
The schema-1 rollout removal is tracked in CI#427. Please re-review the updated PR. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Exact-head deep review passed. I checked the shared producer/consumer contract with matrixorigin/CI#426: the consumer pins the pulled builder by immutable digest, computes the fingerprint inside that same image, restores only thirdparties/install on an exact schema-3 (or rollout-only legacy tree) match, and otherwise removes it before a source rebuild. The schema-3 input closure covers the current thirdparty tree, root/cgo native contract, target OS/arch, and fixed image toolchain; generated outputs are excluded. The legacy comparison is a full Git tree identity and is tracked for removal in CI#427. The configurable runtime base is guarded by ldconfig plus a real mo-service load, and the GOPROXY quoting is correct. Shell syntax, deterministic repeated fingerprint output, diff check, and the exact-head required CI set pass. No P0/P1/P2 correctness, lifecycle, or stale-artifact blocker found.
Merge Queue Status
This pull request spent 58 minutes in the queue, including 57 minutes 24 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27333
What this PR does / why we need it:
optools/images/ci-builder-fingerprint.sh, the canonical native-build contract shared by the Basic Image producer and CI consumers.Dockerfile.ci-builderto emit the new fingerprint, with a schema-1 compatibility fallback for CI/MatrixOne merge-order safety.The companion CI change is matrixorigin/CI#426. It consumes this fingerprint in TKE, restores prebuilt native dependencies only on an exact match, and falls back to source builds otherwise.