feat(ci): pin a single torch-spyre commit across all jobs in a run - #463
feat(ci): pin a single torch-spyre commit across all jobs in a run#463BenjSz wants to merge 8 commits into
Conversation
torch-spyre is baked into each runner pod image, so matrix jobs landed on pods at different HEADs and one run mixed several torch-spyre commits, making results incomparable (#449). Resolve origin/main to one SHA per run, check that exact commit out before uv add in every job, assert each job's HEAD matches, and report the pinned + hf-adapters SHAs in the run summary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Sznajder <benjams@il.ibm.com>
Signed-off-by: Benjamin Sznajder <benjams@il.ibm.com>
arielge
left a comment
arielge
left a comment
There was a problem hiding this comment.
IIUC, this would break the integration test workflow - the integration infrastructure ensures that tests run on an image with a specific torch-spyre version (e.g., of a torch-spyre PR that triggers it), but then here we do git checkout --force "$SHA" which overrides that with a different torch-spyre version
I see - thus, the logic should be that the torch-spyre version "that is triggerring" the weekly should be passed to the different shard, afterthat... |
yup we can use the same pattern as the torch-spyre repo |
The issue #449 pin unconditionally checked out origin/main into the baked torch-spyre, clobbering the version the integration image was built to test. Add a prebaked_image opt-out: integration-tests passes it true, which skips resolve-torch-spyre, the per-job checkout, and the torch-spyre reinstall entirely, so the run uses the image's baked-in venv as-is. PR/daily/weekly keep pinning origin/main (default false). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Benjamin Sznajder <benjams@il.ibm.com>
HarikrishnanBalagopal
left a comment
HarikrishnanBalagopal
left a comment
There was a problem hiding this comment.
Reviewed the full diff (5 files, all .github/). This is a solid, well-documented fix for #449 and it directly answers @arielge's CHANGES_REQUESTED concern: integration-tests.yaml now passes prebaked_image: true, which skips resolve-torch-spyre and the git checkout --force, so an integration run keeps the exact torch-spyre baked into its (per-PR) image instead of overwriting it with origin/main. Verified on the branch head:
.github/workflows/integration-tests.yaml:225setsprebaked_image: true; every other caller omits it (defaultfalse= pinorigin/mainonce).resolve-torch-spyre.if: !inputs.skip_tests && !inputs.prebaked_imageskips the job on that path, so itstorch_spyre_shaoutput is'',build-hf-adapterstakes its unpinned/prebaked branch, andgather-runner-info's pin assertion no-ops.
One substantive gap and two smaller points.
1. spyre-model-components-tests is left unpinned on the default path (partial #449 miss).
_test_matrix.yaml:791 spyre-model-components-tests (and its -retry at :1982) pass prebaked_image but NOT torch_spyre_sha, and the job has needs: [resolve-test-type] only (no resolve-torch-spyre), no pinned_torch_spyre_sha verify. It runs on ${{ inputs.image_label }} (the spyre runner that has the baked /home/senuser/torch-spyre) and is in the unit / integration / regression / trunk tiers (_test_matrix.yaml:794). So on the default pinned path, this one suite still installs whatever HEAD its pod's image baked in, while every sibling suite installs the resolved SHA. That is the exact within-run inconsistency #449 sets out to remove. The inline comment ("This job does not pin torch-spyre... runs on the image's baked-in torch-spyre") states the behavior but not why this suite is exempt. Could you either pin it like the others (add resolve-torch-spyre to needs, pass torch_spyre_sha + pinned_torch_spyre_sha/hf_adapters_sha), or, if it genuinely must stay on baked-in torch-spyre, spell out the reason so a reader does not read it as an oversight?
2. Retry jobs use a different gate than the primary jobs, worth confirming it is intentional.
The primary suite jobs guard with (needs.resolve-torch-spyre.result == 'success' || 'skipped') && ... so a pin FAILURE skips the matrix rather than running on baked-in torch-spyre. The -retry jobs instead keep if: !cancelled() && needs.collect-failed-suites.outputs.has_failed_* == 'true' with resolve-torch-spyre only added to needs. With !cancelled(), a retry job is not implicitly success()-gated, so if resolve-torch-spyre failed a retry could still fire on baked-in torch-spyre. In practice the primary jobs skip on pin failure, so nothing runs, so has_failed_* stays false and the retries do not fire, but the fail-closed property here is emergent from collect-failed-suites rather than explicit. A one-line comment noting that (or mirroring the result == guard) would make the two paths read consistently. Not a blocker.
3. git fetch ... origin "${TORCH_SPYRE_SHA}" relies on server-side fetch-by-SHA.
build-hf-adapters/action.yaml fetches the bare 40-char SHA, with a full-fetch fallback (|| git fetch --tags --force origin). Fetch-by-SHA needs uploadpack.allowReachableSHA1InWant/allowAnySHA1InWant on the remote; if the server rejects it the fallback covers it, so this is fine. Just flagging that the fallback is load-bearing, not cosmetic, in case someone trims it later.
Nits already left inline: uv pip show --active / uv pip freeze --active on the prebaked path (--active should be harmless for a show/freeze but is not needed).
Test plan question: the PR body's test plan is unchecked-in-spirit for the integration path. Since the biggest behavioral change is the new prebaked_image=true branch that #449's original patch did not have, has an actual integration-tests run been dispatched on a per-PR image to confirm the suites use the image's torch-spyre and do NOT re-resolve? A dispatched push-to-clickhouse (small top_k, dev db) plus one integration-tests run, with the per-job torch-spyre HEAD markers pasted, would close the loop on the exact regression @arielge raised.
remove useless --active in uv pip freeze | show
…fail-closed Address PR #463 review (issue #449): 1. spyre-model-components-tests (+ -retry) was the one suite left unpinned on the default path -- it ran on its pod's baked-in torch-spyre HEAD while every sibling suite installed the resolved SHA, the exact within-run inconsistency #449 removes. The suite exercises torch-spyre (moves tensors to Spyre, requires_spyre), so there is no reason to exempt it. Pin it like the others: add resolve-torch-spyre to needs, gate on its result, pass torch_spyre_sha to build, and add the pinned_torch_spyre_sha/hf_adapters_sha pin-verify to gather-runner-info. On the prebaked integration path resolve-torch-spyre is skipped, torch_spyre_sha is '' and the verify no-ops, matching token_compare/embed_compare exactly. 2. Document why the *-retry jobs use !cancelled() rather than the primary jobs' result == guard: fail-closed on a pin failure is emergent (a failed pin skips every primary, so nothing runs, so has_failed_* stays false and the retry never fires). Full note on spyre-load-tests-retry; short pointer on spyre-model-components-tests-retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review @HarikrishnanBalagopal. Addressed in 129a3f4: 1.
On the prebaked integration path ( 2. Retry gate vs. primary gate — documented rather than changed. 3. Load-bearing
On the test-plan question: I haven't dispatched the |
Signed-off-by: Benjamin Sznajder <benjams@il.ibm.com>
|
⚙️ Auto-merge enabled — dispatched Automated: enabling auto-merge requests the gate, so the PR is not left waiting on a check nobody asked for. Re-enabling auto-merge on this same commit will not dispatch again. |
✅ spyre-test: successTriggered by: /spyre-test comment Plan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🟢<br/>image · a39b59f6076f"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev 🟢<br/>image · a225402a774e"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev 🟢<br/>image · 2fa8181dccc9"]
end
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_torch_spyre_torch_spyre_dev sOk;
class n_amd64_hf_adapters_hf_adapters_dev sOk;
class n_amd64_spyre_inference_spyre_inference_dev sOk;
✅ orch trigger-pr-validation — green · arches amd64 · fp amd64=8a452b82
GHA test runs:
✅ safe to mergeBuild: built 3 Tests: passed 6 · blocking 0 · advisory 0 · infra/inconclusive 0 · no signal 0 Before merging, consider:
|
|
GHA test runs:
|
|
GHA test runs:
|
|
GHA test runs:
|
❌ merge-queue-integration: failurePlan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🔴<br/>image · 16ec4926d4c8"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev<br/>image · 9e02b8719045"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev<br/>image · 86eca61204b1"]
end
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_torch_spyre_torch_spyre_dev sOk;
class n_amd64_hf_adapters_hf_adapters_dev sFailed;
class n_amd64_spyre_inference_spyre_inference_dev sDropped;
GHA test runs:
❌ build failed (1) — do not mergeBuild: built 1 · failed 1 · dropped 1
Tests: passed 0 · blocking 0 · advisory 2 · infra/inconclusive 0 · no signal 0 Failures by kind
Before merging, consider:
|
|
GHA test runs:
|
❌ merge-queue-integration: failurePlan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🔴 ↺<br/>image · fc1752087216"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev 🟢<br/>image · 5e4624f9f429"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev 🔴<br/>image · 75f3523c474b"]
end
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_torch_spyre_torch_spyre_dev sReused;
class n_amd64_hf_adapters_hf_adapters_dev sOk;
class n_amd64_spyre_inference_spyre_inference_dev sOk;
✅ orch trigger-pr-validation — green · arches amd64 · fp amd64=add923f9
GHA test runs:
|
|
GHA test runs:
|
|
GHA test runs:
|
|
GHA test runs:
|
3 participants
Summary
Fixes #449. A single CI run installed different
torch-spyrecommits across its jobs, becausetorch-spyreis a git checkout baked into each runner pod image at/home/senuser/torch-spyreand matrix jobs land on pods built at different times (the issue's example: 75 weekly-scan jobs spread across 6 commits). Results within one run were therefore not comparable or reproducible.This resolves the
torch-spyrerevision once per run and pins it everywhere:build-hf-adapters/action.yaml— new optionaltorch_spyre_shainput. When set, the baked checkout is fetched + checked out at exactly that commit beforeuv add(repurposes the previously-deadINPUT_REFbranch). Empty (default) = today's behaviour, so other callers are unaffected.gather-runner-info/action.yml— newpinned_torch_spyre_sha/hf_adapters_shainputs. Asserts each job's checked-out torch-spyreHEADequals the pinned SHA (fails the job on mismatch — the per-shard "verify it's pinned" check) and reports both SHAs in the run summary._test_matrix.yaml(PR / daily / integration) — newresolve-torch-spyrejob resolvesorigin/mainto one SHA on a spyre runner, threaded into all 24 suite + retry jobs and guarded by therun spyre unit testsgate so a resolve failure can't produce a false green.push-to-clickhouse.yaml(the weekly scan — the issue's example run) — its ownresolve-torch-spyrejob (itsgenerate-matrixruns on a backend runner without the checkout), threaded into the threeweekly-model-scan-x1/-x2/-x4fan-out jobs with a post-build pin-verify step.hf-adapters SHA is already deterministic per run (PR head /
github.sha); this change reports it alongside the pinned torch-spyre SHA rather than re-pinning it. Recording either commit in ClickHouse is a schema change and is out of scope.Test plan
actionlintpasses on both workflows and the full.github/workflowsdirpre-commit(check-yaml, whitespace, EOF, uv.lock) passes on all four filespush-to-clickhouserun (smalltop_k,clickhouse_db: dev) and atest_pull_requestrun; confirm every job's--- torch-spyre HEAD ---marker shows the identical SHA and the run summary shows the pinned SHA once🤖 Generated with Claude Code