Skip to content

KernelAbstractions 0.10 compat: admit it, keep the 0.9 contract exact - #133

Merged
sshin23 merged 5 commits into
mainfrom
ka-0.10-compat
Oct 13, 2026
Merged

sshin23 merged 5 commits into
mainfrom
ka-0.10-compat

Conversation

@sshin23

@sshin23 sshin23 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

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):

78k condensed KKT, CPU backend KA 0.9.45 KA 0.10.0
refactorization 1554 / 1553 ms 1270 / 1323 ms (-17%)
solve 138 / 143 ms 106 / 115 ms (-21%)
full CPU suite wall (32 workers, all four eltypes) 14m39s 5m53s

What 0.10 needed (none of it numerical)

  • Per-launch host allocation (~230 B measured with a trivial kernel; 0.9 was allocation-free): ka_cpu_alloc_budget carries 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.
  • Caught in-kernel exceptions are announced on stderr (noise in the negative tests; harmless, tests unaffected).
  • Worker tasks stay alive at exit, so Aqua's persistent-tasks check reports the backend rather than the package: skipped on >= 0.10 with the upstream reason documented; unchanged on 0.9.

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

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>
@sshin23 sshin23 added the claude:pr PR opened by the Claude implementer; handled by the Claude pipeline label Oct 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

CI-FIX round 1 of 3: https://github.com/exanauts/SparseDirectSolver.jl/actions/runs/38424176322 failed on b190d09; Claude is diagnosing it.

Comment thread test/utils.jl Outdated
Comment on lines +492 to +495
# ~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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and abs_max! reductions are the + 2. The extra launches come from the regime-C / wide regime-B groups that take _factorize_ldlt_c_group! (one panel_ldlt_kernel! per block of nb pivot 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.

Comment thread test/test_aqua.jl Outdated
Comment on lines +12 to +16
# 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,)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread test/utils.jl
Comment on lines +250 to +255
# 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. .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>
@claude

claude Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Review round 1 — addressed in dc89cd6

Blocking (Aqua workflow runs persistent-tasks under KA 0.10): fixed, but I could not push the workflow part. The owner has to apply it.
The GitHub App cannot push to .github/workflows/ ("refusing to allow a GitHub App to create or update workflow .github/workflows/Aqua.yml without workflows permission"). So the Aqua check stays red on this head until the patch below is applied. I ran the patched command locally in a fresh environment that resolves KA 0.10.0, and every Aqua test passed (persistent tasks skipped). I've labelled the PR needs-owner.

--- 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"))'

test/test_aqua.jl: the reason for the skip now names both failure modes: worker tasks stay alive at exit, and the pocl_standalone_jll / SLEEF_jll precompile error that CI shows. It also notes that the workflow applies the same gate.

Nit, ldlt_alloc_budget: fixed. The budget is now ldlt_alloc_budget(S, N), and it takes the launch count from N.plan the way the driver launches:

  • one launch per subtree launch and per fused regime-A/B group;
  • for each blocked-path group (group_width == 0 or ldlt_blocked_path): 3 assembly launches, plus, for each ldlt_c_chunk_end chunk and batch member, (1 panel + 2 GEMMs per front) per block of nb columns, the finish kernel and one pack per front;
  • plus the two reductions.

This replaces the flat 3 per group. The @localmem coverage term is back to the pre-PR count (fused launches + 2), so the KA 0.9 coverage allowance is no longer inflated. test_numeric_lu.jl had used the LDL helper for factorize_lu!. It now calls ldlt_alloc_budget(S, Nd; lu = true), where every group is one fused launch plus the Schur assembly. The docstring is updated.

Nit, ka_cpu_alloc_budget docstring: fixed. It now documents the 320 B per launch allowed under KA >= 0.10.

No findings rejected. Tests: SDS_TEST_GPU=0 full suite on KA 0.10.0 passes, 72005/72005, all four element types. CUDA and AMDGPU are pending CI. This is a maintenance PR with no TASKS.md Report block, so none was updated.

🤖 Generated with Claude Code

@claude claude Bot added the needs-owner The Claude pipeline stopped; the owner must look label Oct 13, 2026
…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>
Comment thread .github/workflows/Aqua.yml Outdated
# 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))'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

Comment thread test/utils.jl Outdated
end
end
launches = fused + blocked + 2
localmem = (fused + 2) * maximum(SparseDirectSolver.SUBTREE_LOCAL_SIZES) +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. .github/workflows/Aqua.yml line 24: the new run: 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 real build job; and gh pr checks 133 no 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 a run: | block scalar, and confirm the workflow produces a job on the next push. (Inline comment posted.)

Non-blocking

  • test/utils.jl ldlt_alloc_budget: the coverage term now grants blocked groups only 64 B × launches instead of the former 48 KiB per group, while their kernels (panel_ldlt_kernel!, finish_ldlt_kernel!, ka_gemm! tiles, assembly) do have @localmem that 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>
@claude

claude Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Review round 2 (head 73b397b)

Changed:

  • test/utils.jl ldlt_alloc_budget (non-blocking nit 4261497700): the coverage-mode @localmem term again grants 48 KiB per blocked launch group, as before this PR ((fused + nblocked + 2) × 48 KiB), so the KA 0.9 + --code-coverage budget is no tighter than on main. LU: the Schur assembly is counted as two launches; docstring updated.
  • ka_cpu_alloc_budget docstring (nit 4260845073): already lists the KA ≥ 0.10 320 B/launch term since dc89cd6; no change needed.

Blocking finding 4261485075 (Aqua.yml invalid YAML): the fix is not pushed and needs the owner. The push was rejected: refusing to allow a GitHub App to create or update workflow .github/workflows/Aqua.yml without workflows permission. Please apply this one-line change by hand (I checked it parses with yaml.safe_load):

-        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: SDS_TEST_GPU=0 julia --project=. -e 'using Pkg; Pkg.test()' → 72005 / 72005 pass (KA CPU backend only). CUDA: pending CI on the PR.

This is not a TASKS.md task, so there is no Report block to update. I've labelled the PR needs-owner for the workflow edit.

Comment thread .github/workflows/Aqua.yml Outdated
# 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))'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Suggested change
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in src/ or ext/, so the KA kernel rules are unaffected.
  • test/utils.jl ka_cpu_alloc_budget: 320 B/launch only under pkgversion(KernelAbstractions) >= v"0.10", the 0.9 contract (1024 B, no per-launch term) is unchanged; the docstring now lists the term.
  • test/utils.jl ldlt_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 == 0 or ldlt_blocked_path) makes zero_fronts!/scatter_A!/extend_add! (3), then per ldlt_c_chunk_end chunk and member nblocks panel_ldlt_kernel!, at most two :ka GEMM launches per front per block, one finish_ldlt_kernel! and one pack_add! per front; plus abs_max! and reduce_stats!. The helper matches _factorize_ldlt_groups!/_factor_chunk_ldlt_c! exactly (it is an upper bound: N.nbatch instead of active members, GEMMs counted even for blocks past a narrower front). The coverage-mode @localmem term grants 48 KiB per subtree launch, fused group and blocked group, so the KA 0.9 + coverage budget is no tighter than on main. 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_tasks skipped only on KA ≥ 0.10 with both upstream reasons documented; (tmax = 120,) kept on 0.9. KernelAbstractions is in scope through test/backends.jl.
  • CI: on the previous head 2b1ff4c the 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):

  1. .github/workflows/Aqua.yml:24 is invalid YAML (: inside the plain run: scalar from ? false : true)). Run 38437628227 for this head has zero jobs and is reported under the file path, and gh pr checks no longer shows an Aqua check. build is a required status check on main, 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 carries needs-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.

@claude

claude Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

nit (non-blocking), test/utils.jl:535 (ldlt_alloc_budget, could not be anchored inline): the lu ? 2 : 0 term is documented (docstring lines 525–527) as "the two Schur assembly launches", but factorize_lu! only reaches a launch in assemble_schur! when plan.schur_first != 0, i.e. with a Schur root; the one caller with lu = true (test_numeric_lu.jl "determinism and refactorization") uses schur_mode = 0, so assemble_schur! returns before any launch (src/numeric/schur.jl:68), and even with a Schur root the LU path makes a single _schur_assemble_lu_kernel! launch. The LDLᵀ budget gets no such term although its driver calls assemble_schur! too. The budget is an upper bound, so this only adds 640 B of unexplained slack on KA ≥ 0.10 (128 B under coverage). Either drop the term (blocked = 0) or label it as headroom rather than as Schur launches.

@claude

claude Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Fix round 3 (head 73b397b): no new commit was pushed.

  • Blocking: .github/workflows/Aqua.yml:24 is invalid YAML. Accepted. I applied the reviewer's suggested line (persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"), with no : in the plain scalar; it passes yaml.safe_load) and committed it, but GitHub rejected the push: refusing to allow a GitHub App to create or update workflow .github/workflows/Aqua.yml without workflows permission. I dropped the local commit. Owner action needed: replace line 24 with the suggestion in the inline thread, then check that the Aqua build job actually runs.
  • Nit on the ka_cpu_alloc_budget docstring: already fixed at this head (the docstring lists the 320 B/launch term under KA ≥ 0.10).
  • Nit on lu ? 2 : 0 in ldlt_alloc_budget: left unchanged. It is a small upper-bound slack, as the reviewer says, and does not weaken any test.

I did not rerun the test suite, since no code changed in this round. needs-owner stays on the PR.

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>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:24 now reads persistent_tasks=(pkgversion(KernelAbstractions) < v"0.10"), with no : inside the plain scalar. GitHub parses the workflow again: this head has a real build job (pending), whereas the two previous heads produced zero-job parse failures. The gate matches test/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 in src/ or ext/, so the AGENTS.md kernel rules are unaffected.
  • test/utils.jl: ka_cpu_alloc_budget adds 320 B/launch only under KA >= 0.10; the 0.9 budget (1024 B, plus the coverage term) is unchanged. I re-derived ldlt_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) and factorize_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, per ldlt_c_chunk_end chunk and member, nblocks panel launches, at most two GEMMs per front per block, one finish kernel and one pack_add! per front, and abs_max! + reduce_stats! are the + 2. The helper is a correct upper bound (nbatch instead of nact, GEMMs counted for blocks past narrower fronts). The coverage-mode @localmem term grants 48 KiB per subtree launch, fused group and blocked group, so the KA 0.9 + coverage budget is no tighter than on main. No test weakened, no @test_broken.
  • The merge with the current main (which gained #129 meanwhile) is clean: the only callers of ldlt_alloc_budget in the merged tree (test/test_numeric_ldlt.jl:319, test/test_numeric_lu.jl:213) use the new signature. KernelAbstractions is in scope for the test modules through test/runtests.jl:33 and test/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/ and test/).

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.

@sshin23 sshin23 removed the needs-owner The Claude pipeline stopped; the owner must look label Oct 13, 2026
@sshin23
sshin23 merged commit 0a402ff into main Oct 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude:pr PR opened by the Claude implementer; handled by the Claude pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant