Repository navigation
KernelAbstractions 0.10 compat: admit it, keep the 0.9 contract exact - #133
Conversation
KA 0.10.0 runs this package unchanged and FASTER on the CPU backend (78k condensed KKT, same tree, ABBA: refactorization 1554 -> 1270-1323 ms, solve 138-143 -> 106-115 ms; the full CPU suite wall drops 14m39s -> 5m53s). Three 0.10 behaviors needed accommodation, none numerical: * its CPU backend heap-allocates ~230 B per kernel launch where 0.9 was allocation-free: ka_cpu_alloc_budget carries a 320 B/launch allowance on 0.10 only (the budgets still scale with each phase's launch count; the 0.9 zero-allocation contract stays exact). Surfacing that also exposed a real undercount: the LDL factorization makes ~3 launches per group (measured), not 1 -- corrected. * its CPU backend announces caught in-kernel exceptions on stderr (noise in the negative tests; harmless). * it keeps worker tasks alive at exit, so Aqua's persistent-tasks check reports the backend, not the package: skipped on >= 0.10 with the upstream reason documented, unchanged on 0.9. The GPU stacks are unaffected today: every released CUDA.jl/AMDGPU.jl pins KA 0.9, so GPU environments keep resolving 0.9 until upstream ships 0.10 support -- at which point the fused-kernel A/B is worth re-running. Validated: full CPU suite green on BOTH versions (0.9.45: 72006 incl. the persistent-tasks check; 0.10.0: 72005/72005, all four element types). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI-FIX round 1 of 3: https://github.com/exanauts/SparseDirectSolver.jl/actions/runs/38424176322 failed on b190d09; Claude is diagnosing it. |
| # ~3 launches per group measured on the LDL path (subtree kernel + per-group pivot | ||
| # and statistics passes), not 1: the undercount was invisible while KA 0.9 launches | ||
| # were allocation-free, and surfaced through the 0.10 per-launch allowance | ||
| launches = 3 * length(S.schedule.groups) + 2 |
There was a problem hiding this comment.
nit (non-blocking): the rationale in this comment and the docstring above no longer match the driver.
- The docstring (line 487) still says "one launch per launch group"; the code now budgets three.
- The comment attributes the extra launches to "per-group pivot and statistics passes". In
_factorize_ldlt_groups!(src/numeric/ldlt.jl:1416–1441) a regime-A budget class and a regime-B bin are each exactly one launch; the statistics andabs_max!reductions are the+ 2. The extra launches come from the regime-C / wide regime-B groups that take_factorize_ldlt_c_group!(onepanel_ldlt_kernel!per block ofnbpivot columns plus the GEMM kernels), so the count is per-block, not a constant 3 per group — on a matrix with a large regime-C front and few groups the 320 B/launch allowance would be exceeded again.
Suggested fix: derive the count from N.plan / S.schedule (a regime-C group contributes ceil(ncols / nb) panel launches plus its GEMM launches, the others 1) the way solve_alloc_budget walks ws.plan, and update the docstring. Also note that tripling launches triples the coverage-mode localmem allowance on KA 0.9 too, so "the 0.9 contract stays exact" only holds for non-coverage runs; the per-block count would fix that as well.
| # persistent_tasks is skipped under KernelAbstractions >= 0.10: its CPU backend keeps | ||
| # worker tasks alive at exit, so the check reports the backend, not this package | ||
| # (upstream; re-enable when KA's task pool shuts down cleanly) | ||
| Aqua.test_all(SparseDirectSolver; ambiguities = false, undefined_exports = false, | ||
| persistent_tasks = (tmax = 120,)) | ||
| persistent_tasks = (pkgversion(KernelAbstractions) >= v"0.10" ? false : (tmax = 120,))) |
There was a problem hiding this comment.
Blocking: the skip is applied here but not to the Aqua workflow, which runs the persistent-tasks check unconditionally (.github/workflows/Aqua.yml: Aqua.test_all(SparseDirectSolver, ambiguities=false) in a fresh ubuntu-latest environment). With this PR's compat, a CPU-only environment resolves KA 0.10.0 (GPUArrays 12.1.0 and AcceleratedKernels 0.5 both admit it; only the CUDACore/AMDGPU registry entries pin 0.9), so that workflow now goes red on every CPU-only run. CI on this head shows exactly that (build job of run 38424176322):
+ KernelAbstractions v0.10.0
Persistent tasks: Error During Test at .../Aqua/yUvBM/src/persistent_tasks.jl:40
Loading `SparseDirectSolver` for the persistent-task check failed before precompilation completed
ERROR: LoadError: ArgumentError: Cannot load (`using/import`) module SLEEF_jll into module pocl_standalone_jll in package pocl_standalone_jll
Test Summary: | Error Total
Persistent tasks | 1 1
AGENTS.md lists Aqua as a check whose red result triggers CI-fix rounds, so the PR has to leave it green. Fix: apply the same version-gated skip in the workflow command (persistent_tasks = pkgversion(KernelAbstractions) >= v"0.10" ? false : true, loading KernelAbstractions there), or pin the workflow's environment to KA 0.9 explicitly. Note also that the failure mode CI shows is a precompilation error inside Aqua's temporary project (pocl_standalone_jll cannot load SLEEF_jll), not the "worker tasks stay alive at exit" reason documented in this comment; please mention both so the re-enable condition is accurate.
There was a problem hiding this comment.
Addressed in dc89cd6 for test_aqua.jl (skip reason now names both failure modes). The matching Aqua.yml gate is written and verified locally, but the GitHub App cannot push workflow files (no workflows permission); the patch is in the PR comment for the owner, PR labelled needs-owner.
| # KernelAbstractions 0.10's CPU backend heap-allocates per launch (~230 B measured | ||
| # with a trivial precompiled kernel; the new backend's task machinery), where 0.9 | ||
| # was allocation-free. 320 B of per-launch headroom keeps the budgets meaningful -- | ||
| # they still scale with the phase's launch count -- until the upstream allocation | ||
| # is fixed; the 0.9 contract stays exact. | ||
| pkgversion(KernelAbstractions) >= v"0.10" && (b += 320 * launches) |
There was a problem hiding this comment.
nit (non-blocking): the docstring of ka_cpu_alloc_budget (lines 237–247) still states "0 without coverage" as the measured per-launch cost and only lists the coverage and Julia ≤ 1.11 allowances; the KA ≥ 0.10 320 B/launch term lives only in this code comment. Move a one-line mention into the docstring so the budget's three terms are documented in one place (it is the helper the task reports cite).
There was a problem hiding this comment.
VERDICT: CHANGES_REQUESTED (head b190d09)
Reviewed the five-file diff (Project.toml, test/Project.toml, test/test_aqua.jl, test/utils.jl, AGENTS.md) against AGENTS.md and the CI state. This is a maintenance PR by the owner, not a TASKS.md task, so there is no Report block; PLAN.md and TASKS.md are untouched, no Manifest.toml, no public names changed.
Checked and confirmed:
- The dependency claims in the PR body hold: registered CUDACore 6.x pins KA 0.9.38 - 0.9 and AMDGPU 2.x pins 0.9.2 - 0.9, so GPU environments keep 0.9; GPUArrays 12.1.0 and AcceleratedKernels 0.5 admit 0.10, so CPU-only environments now resolve KA 0.10.0.
- Aqua.test_all accepts persistent_tasks = false (checked the Aqua source), and KernelAbstractions is in scope for pkgversion in both test files via init_code.
- ka_cpu_alloc_budget leaves the KA 0.9 non-coverage budget at 1024 B unchanged; the 0.10 term scales with the launch count as described.
Blocking finding (inline on test/test_aqua.jl):
- .github/workflows/Aqua.yml runs the persistent-tasks check unconditionally in a fresh ubuntu-latest environment, which with this compat resolves KA 0.10.0. The Aqua build check on this head is red for exactly that reason (precompilation error inside the temporary project of Aqua: pocl_standalone_jll cannot load SLEEF_jll). AGENTS.md lists Aqua as a check that triggers CI-fix rounds, so the PR must carry the same version-gated skip (or an explicit KA 0.9 pin) in that workflow, and the documented skip reason should mention the failure mode CI actually shows.
Non-blocking nits (inline on test/utils.jl): the ldlt_alloc_budget docstring and comment do not match the driver (the extra launches come from the regime-C blocked path per block, not from per-group pivot/stat passes, and tripling launches also triples the coverage-mode localmem allowance on 0.9); the ka_cpu_alloc_budget docstring should mention the new 0.10 term.
CI: the Run tests jobs (cpuonly, cuda, amdgpu) and the documentation build were still in progress at review time; their outcomes are handled by the pipeline.
* ldlt_alloc_budget(S, N): count the launches from N.plan (blocked-path groups per chunk and block of nb pivot columns) instead of a constant 3 per group; the coverage-mode localmem term is back to the pre-PR fused-launch count. lu = true for factorize_lu!. * ka_cpu_alloc_budget docstring lists the KA >= 0.10 per-launch term. * test_aqua.jl: the persistent-tasks skip reason names both failure modes (tasks alive at exit; pocl_standalone_jll precompile failure seen on CI). The matching gate in .github/workflows/Aqua.yml cannot be pushed by the GitHub App (no workflows permission); the patch is on the PR for the owner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 — addressed in dc89cd6Blocking (Aqua workflow runs persistent-tasks under KA 0.10): fixed, but I could not push the workflow part. The owner has to apply it. --- a/.github/workflows/Aqua.yml
+++ b/.github/workflows/Aqua.yml
@@ -17,4 +17,7 @@ jobs:
# MadNLP is loaded so the SparseDirectSolverMadNLPExt extension assigns the
# exported SDSSolver/SDSSolverOptions globals: the undefined-exports check
# then validates them for real instead of flagging the declared-only state
- run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP"]); Pkg.develop(path="."); using Aqua, MadNLP, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false)'
+ # persistent_tasks is skipped under KernelAbstractions >= 0.10, as in test/test_aqua.jl
+ # (KA 0.10's CPU backend keeps tasks alive at exit, and its pocl_standalone_jll fails to
+ # precompile in Aqua's temporary project): the check would report the backend
+ run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"))'
Nit,
This replaces the flat 3 per group. The Nit, No findings rejected. Tests: 🤖 Generated with Claude Code |
…0.10 The review's blocking finding: test/test_aqua.jl gained the version-gated skip but the standalone Aqua workflow still ran the check unconditionally, and its fresh ubuntu-latest environment resolves KA 0.10.0, so the build job went red (pocl_standalone_jll fails to precompile in Aqua's temporary project: "Cannot load SLEEF_jll"). Apply the same gate there, with both upstream failure modes documented at both sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| # two upstream reasons as in test/test_aqua.jl: KA 0.10's CPU backend keeps | ||
| # worker tasks alive at exit, and Aqua's fresh temporary project fails to | ||
| # precompile pocl_standalone_jll ("Cannot load SLEEF_jll"). | ||
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) >= v"0.10" ? false : true))' |
There was a problem hiding this comment.
Blocking: this line makes Aqua.yml invalid YAML, so the Aqua workflow no longer runs at all.
The run: value is a plain (unquoted) scalar, and plain scalars may not contain : (colon-space): the ? false : true) you added is parsed as a mapping indicator ("mapping values are not allowed in this context"). The single quotes around the Julia code do not protect it, since they are just characters inside the plain scalar.
Evidence on this head: workflow run 38432641415 for 2b1ff4c is reported as .github/workflows/Aqua.yml (the file path, not the workflow name), completed / failure, with zero jobs, which is GitHub's signature for a workflow-file parse error. The previous head dc89cd6 (old Aqua.yml) ran a real build job that failed inside the Aqua step with the SLEEF_jll/persistent-tasks error you documented. Also note gh pr checks 133 no longer lists an Aqua check for this head, so the broken workflow would not even show up as red.
Fix: drop the ternary so the scalar has no : sequence, e.g.
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) >= v"0.10" ? false : true))' | |
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"))' |
(or move the command into a block scalar run: |). Please confirm the Aqua workflow actually produces a job on the next push.
| end | ||
| end | ||
| launches = fused + blocked + 2 | ||
| localmem = (fused + 2) * maximum(SparseDirectSolver.SUBTREE_LOCAL_SIZES) + |
There was a problem hiding this comment.
nit (non-blocking): the coverage term of this budget is now tighter than before for blocked groups, which is the one combination (KA 0.9 + --code-coverage) no CI configuration exercises any more (the CPU-only CI job resolves 0.10; the CUDA/AMDGPU jobs skip the CPU backend).
Before, localmem granted 48 KiB per launch group (length(S.schedule.groups) + 2), blocked groups included. Now only the subtree and fused launches get 48 KiB, while a blocked group gets just 64 B × launches. Under KA 0.9 with coverage the @localmem MArrays of the blocked-path launches are heap-allocated too: panel_ldlt_kernel! (ctl, pv, red = 3·WG reals, redi = 2·WG ints, per block), finish_ldlt_kernel! (ctl), the ka_gemm! tiles (2·TILE² of T, per GEMM), and the assembly kernels. The test matrices (regime_c_width = 16, regime_c_rows = 128) do take the blocked path, so a 0.9 + coverage run of test_numeric_ldlt/test_numeric_lu is likely to exceed the new budget, which contradicts "the 0.9 contract stays exact" for that configuration.
Suggested fix: keep the blocked launches' local memory in the coverage term, e.g. add per blocked group the panel/finish/GEMM @localmem sizes (or keep the former 48 KiB per launch group for localmem while using the new launches count for the headers). Minor, same docstring: the LU budget's "plus the Schur assembly" is 2 launches with a Schur root (_schur_assemble_lu_kernel! + _schur_meta_kernel!), not 1; harmless here since the LU test has no Schur root.
There was a problem hiding this comment.
VERDICT: CHANGES_REQUESTED (head 2b1ff4c)
Checked. This is an owner infrastructure PR, not a TNN task of TASKS.md (no matching task exists; the title has no task prefix and there is no Report block), so I reviewed it against AGENTS.md and the diff rather than a task section. PLAN.md, TASKS.md and Manifest.toml are untouched; the only non-code edit is the AGENTS.md dependency note, which matches the new compat. The compat widening (KernelAbstractions = "0.9, 0.10" in both Project.toml and test/Project.toml) is the whole library-side change; no kernel, layout or numeric code moves, so the AGENTS.md kernel/convention rules are unaffected.
I verified the new ldlt_alloc_budget(S, N; nb, lu) against the drivers: _factorize_ldlt_groups! (one launch per plan.sub_first class and per non-blocked group, blocked condition identical to the drivers W == 0 || ldlt_blocked_path), _factorize_ldlt_c_group! (zero_fronts!+scatter_A!+extend_add!= 3, chunks by the sameldlt_c_chunk_endcall, per member and chunknblockspanel launches, at most twoka_gemm!launches per front per block, one finish kernel, onepack_add!per front),abs_max!+reduce_stats!(= 2), andfactorize_lu!(one launch per subtree class and per group). The formula is a correct upper bound on the launch count (it usesnbatchwhere the driver loops overnact, and counts abs_max!unconditionally).pkgversion(KernelAbstractions)resolves in the test modules through theusing KernelAbstractionspreamble oftest/runtests.jl, so test_aqua.jlandutils.jlare fine. No test was weakened: under KA 0.9 without coverage the budgets are unchanged (1024 B), and the 0.10 allowance scales with launches. Thetest_aqua.jl gate is documented with the upstream reason, and the dc89cd6 Aqua log confirms it (Cannot load SLEEF_jll into pocl_standalone_jll from Aquas temporary project under KA 0.10.0).
CI: Run tests is green on dc89cd6 (identical src/ and test/ to this head): the CPU-only job ran KA 0.10.0 with coverage, and the CUDA and AMDGPU jobs (KA 0.9) are green. The body`s validation claims are consistent with that.
Blocking findings
.github/workflows/Aqua.ymlline 24: the newrun:value is a plain YAML scalar containing:(the? false : true)ternary), which is not allowed; the workflow file no longer parses. Evidence: the Aqua run for this head (38432641415) is reported under the file path,completed/failure, with zero jobs, whereas the previous head had a realbuildjob; andgh pr checks 133no longer lists an Aqua check for this head at all. Fix: write the gate without a ternary, e.g.persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"), or use arun: |block scalar, and confirm the workflow produces a job on the next push. (Inline comment posted.)
Non-blocking
test/utils.jlldlt_alloc_budget: the coverage term now grants blocked groups only64 B × launchesinstead of the former 48 KiB per group, while their kernels (panel_ldlt_kernel!,finish_ldlt_kernel!,ka_gemm!tiles, assembly) do have@localmemthat is heap-allocated under KA 0.9 with coverage. No CI configuration exercises 0.9 + coverage any more, but the PR claims the 0.9 contract stays exact. Suggested fix in the inline comment. Also the LU docstring`s "Schur assembly" is two launches with a Schur root, not one (harmless for the current tests).
ldlt_alloc_budget: keep the 48 KiB coverage-mode localmem allowance per blocked group as before; the LU Schur assembly is two launches. (The Aqua.yml YAML fix needs the owner: the App token lacks the workflows permission.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review round 2 (head 73b397b) Changed:
Blocking finding 4261485075 (Aqua.yml invalid YAML): the fix is not pushed and needs the owner. The push was rejected: - run: julia --color=yes -e '... Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) >= v"0.10" ? false : true))'
+ run: julia --color=yes -e '... Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"))'No findings rejected. Tests: This is not a TASKS.md task, so there is no Report block to update. I've labelled the PR |
| # two upstream reasons as in test/test_aqua.jl: KA 0.10's CPU backend keeps | ||
| # worker tasks alive at exit, and Aqua's fresh temporary project fails to | ||
| # precompile pocl_standalone_jll ("Cannot load SLEEF_jll"). | ||
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) >= v"0.10" ? false : true))' |
There was a problem hiding this comment.
Blocking (unchanged since round 2; needs the owner's push): this line is invalid YAML, so the Aqua workflow does not run at all.
The run: value is a plain (unquoted) block scalar, and a plain scalar may not contain : (colon-space). The ? false : true) inside the Julia code is read as a mapping indicator ("mapping values are not allowed in this context"); the single quotes are just characters of the plain scalar and do not protect it.
Evidence on this head: workflow run 38437628227 for 73b397b is reported under the file path .github/workflows/Aqua.yml instead of the workflow name, completed / failure, with zero jobs (same signature as run 38432641415 on 2b1ff4c). gh pr checks 133 lists no Aqua check any more. Since build (this workflow's job) is a required status check on main, the PR cannot merge in this state, and merging it by hand would break the Aqua workflow for every later push.
Fix (one line, no colon-space; the ternary is unnecessary since the comparison is already the Bool):
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) >= v"0.10" ? false : true))' | |
| run: julia --color=yes -e 'using Pkg; Pkg.add(["Aqua", "MadNLP", "KernelAbstractions"]); Pkg.develop(path="."); using Aqua, MadNLP, KernelAbstractions, SparseDirectSolver; Aqua.test_all(SparseDirectSolver, ambiguities=false, persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"))' |
(Alternatively wrap the whole value in a block scalar run: >- or double quotes.) The GitHub App cannot push .github/workflows/, so this has to be applied by the owner; everything else in the PR is fine.
There was a problem hiding this comment.
Agreed; I applied exactly the suggested line (persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"), which passes yaml.safe_load). The push was rejected: refusing to allow a GitHub App to create or update workflow .github/workflows/Aqua.yml without workflows permission. So the owner has to apply this one-line change; the PR keeps needs-owner.
There was a problem hiding this comment.
VERDICT: CHANGES_REQUESTED (head 73b397b)
What I checked. Owner infrastructure PR (not a TNN task: no TASKS.md section, no Report block). PLAN.md, TASKS.md, Manifest.toml untouched; no public names, parameter or phase strings changed; the only prose edit is the AGENTS.md dependency note, which matches the new compat. Reviewed the eight-file diff against AGENTS.md, the LDLᵀ/LU drivers and the prior review rounds.
Project.toml/test/Project.toml:KernelAbstractions = "0.9, 0.10"; no code change insrc/orext/, so the KA kernel rules are unaffected.test/utils.jlka_cpu_alloc_budget: 320 B/launch only underpkgversion(KernelAbstractions) >= v"0.10", the 0.9 contract (1024 B, no per-launch term) is unchanged; the docstring now lists the term.test/utils.jlldlt_alloc_budget(S, N; nb, lu): I re-derived the launch count from the drivers. Subtree launches (plan.sub_first) and fused regime-A/B groups are one launch each; a blocked group (group_width == 0orldlt_blocked_path) makeszero_fronts!/scatter_A!/extend_add!(3), then perldlt_c_chunk_endchunk and membernblockspanel_ldlt_kernel!, at most two:kaGEMM launches per front per block, onefinish_ldlt_kernel!and onepack_add!per front; plusabs_max!andreduce_stats!. The helper matches_factorize_ldlt_groups!/_factor_chunk_ldlt_c!exactly (it is an upper bound:N.nbatchinstead of active members, GEMMs counted even for blocks past a narrower front). The coverage-mode@localmemterm grants 48 KiB per subtree launch, fused group and blocked group, so the KA 0.9 + coverage budget is no tighter than onmain. Both callers (test_numeric_ldlt.jl:319,test_numeric_lu.jl:213) pass the device numeric; no test weakened, no@test_broken.test/test_aqua.jl:persistent_tasksskipped only on KA ≥ 0.10 with both upstream reasons documented;(tmax = 120,)kept on 0.9.KernelAbstractionsis in scope throughtest/backends.jl.- CI: on the previous head
2b1ff4cthe CPU-only job (resolves KA 0.10.0) and the CUDA job are green; the round-2 budget changes only loosen the budget. CI on this head is still pending and is left to the pipeline. The PR body's counts (72005 on 0.10, 72006 on 0.9: the persistent-tasks test) are consistent with the skip.
Blocking finding (one, carried over from round 2, unchanged at this head):
.github/workflows/Aqua.yml:24is invalid YAML (:inside the plainrun:scalar from? false : true)). Run 38437628227 for this head has zero jobs and is reported under the file path, andgh pr checksno longer shows an Aqua check.buildis a required status check onmain, so the PR cannot merge, and merging would break the workflow for every later push. Fix:persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10")(inline comment with the exact line). The GitHub App cannot push workflow files, so this needs the owner; the PR already carriesneeds-owner.
Non-blocking: one nit on the lu ? 2 : 0 term of ldlt_alloc_budget (documented as Schur launches that the LU test never makes; harmless slack).
Once the workflow line is fixed by the owner, the rest of this head is approvable as is.
|
nit (non-blocking), |
|
Fix round 3 (head 73b397b): no new commit was pushed.
I did not rerun the test suite, since no code changed in this round. |
The previous gate's ' ? false : true' put ': ' inside the plain YAML scalar, which is invalid YAML: the Aqua workflow run had zero jobs on the last two heads and the check silently vanished from the PR instead of failing. Same semantics via comparison: persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"). Parse-checked the YAML and the Julia expression locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
VERDICT: APPROVE (head 7811198)
What I checked. Owner infrastructure PR, not a TNN task (no TASKS.md section, no Report block). PLAN.md, TASKS.md and Manifest.toml untouched; no public names, parameter or phase strings changed; the only prose edit is the AGENTS.md dependency note, which matches the new compat. The eight-file diff against main is the same as at the previously reviewed head 73b397b except for one line: the branch delta 73b397b..7811198 is exactly the run: line of .github/workflows/Aqua.yml.
- The round-3 blocking finding is fixed.
.github/workflows/Aqua.yml:24now readspersistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"), with no:inside the plain scalar. GitHub parses the workflow again: this head has a realbuildjob (pending), whereas the two previous heads produced zero-job parse failures. The gate matchestest/test_aqua.jl(skip only under KA >= 0.10,(tmax = 120,)kept on 0.9) and the skip reasons are documented in both places. Project.toml/test/Project.toml:KernelAbstractions = "0.9, 0.10"; no change insrc/orext/, so the AGENTS.md kernel rules are unaffected.test/utils.jl:ka_cpu_alloc_budgetadds 320 B/launch only under KA >= 0.10; the 0.9 budget (1024 B, plus the coverage term) is unchanged. I re-derivedldlt_alloc_budget(S, N; nb, lu)against_factorize_ldlt_groups!(src/numeric/ldlt.jl:1416-1441),_factorize_ldlt_c_group!/_factor_chunk_ldlt_c!(src/numeric/ldlt_c.jl:485-567) andfactorize_lu!(src/numeric/lu.jl:560-581): subtree classes and fused groups are one launch each, the blocked condition is the driver's (group_width == 0 || ldlt_blocked_path), a blocked group makes 3 assembly launches plus, perldlt_c_chunk_endchunk and member,nblockspanel launches, at most two GEMMs per front per block, one finish kernel and onepack_add!per front, andabs_max!+reduce_stats!are the+ 2. The helper is a correct upper bound (nbatchinstead ofnact, GEMMs counted for blocks past narrower fronts). The coverage-mode@localmemterm grants 48 KiB per subtree launch, fused group and blocked group, so the KA 0.9 + coverage budget is no tighter than onmain. No test weakened, no@test_broken.- The merge with the current
main(which gained #129 meanwhile) is clean: the only callers ofldlt_alloc_budgetin the merged tree (test/test_numeric_ldlt.jl:319,test/test_numeric_lu.jl:213) use the new signature.KernelAbstractionsis in scope for the test modules throughtest/runtests.jl:33andtest/backends.jl:10. - The PR body's validation claims are consistent with the earlier CI evidence (CPU-only job on KA 0.10.0 and the CUDA/AMDGPU jobs on KA 0.9 green at dc89cd6, identical
src/andtest/).
Blocking findings: none.
Non-blocking, carried over: the lu ? 2 : 0 term in ldlt_alloc_budget is documented as the two Schur assembly launches but is only slack (the one lu = true caller uses schur_mode = 0); the implementer left it as upper-bound headroom, which is acceptable.
CI on this head (Run tests cpuonly/cuda/amdgpu, Aqua build, documentation) is still pending and is left to the pipeline. Note for the owner: the PR still carries needs-owner from the rounds where the workflow edit could not be pushed; now that it is in, that label is what keeps the pipeline from merging.
KernelAbstractions 0.10.0 (released 2026-10-12) runs this package unchanged and measurably faster on the CPU backend. Measured on shin-compute-001 (same tree, same matrix, ABBA interleaving, medians of 7):
What 0.10 needed (none of it numerical)
ka_cpu_alloc_budgetcarries a 320 B/launch allowance on >= 0.10 only — budgets still scale with each phase's launch count, and the 0.9 zero-allocation contract stays exact. Surfacing this exposed a genuine undercount: the LDL factorization makes ~3 launches per group (measured), not 1 — corrected.Scope and risk
GPU environments are unaffected today: every released CUDA.jl/AMDGPU.jl pins KA 0.9, so resolution keeps 0.9 wherever a GPU stack is present. CPU-only users get the faster backend. When CUDA.jl ships 0.10 support, the fused-GPU A/B is worth re-running — if the CPU gains carry over at all, that is free performance.
Validation
Full CPU suite green on BOTH versions, all four element types: 0.9.45 — 72006 pass (incl. the persistent-tasks check); 0.10.0 — 72005/72005 in 5m53s. Targeted re-runs of the budget-bearing files green on both. The per-launch slope, the 3-launches-per-group count, and the exception-banner behavior were each measured directly, not inferred.
🤖 Generated with Claude Code