Fix solve!(cache) type instability + JET QA test group#984
Open
ChrisRackauckas-Claude wants to merge 4 commits into
Open
Fix solve!(cache) type instability + JET QA test group#984ChrisRackauckas-Claude wants to merge 4 commits into
ChrisRackauckas-Claude wants to merge 4 commits into
Conversation
`solve!(cache)` where `cache = init(LinearProblem(A, b))` was returning
`LinearSolution{_A, _B, _C, _D, DefaultLinearSolver, _E, _F} where {...}`
— a UnionAll over 6 free type parameters instead of a concrete
LinearSolution.
Two stacked causes:
1. `do_factorization(::QRFactorization, A, b, u)` returned a `Union` for
`QRFactorization{ColumnNorm}` because the non-inplace branch called
`qr(A)` (dropping the pivot, returning `QRCompactWY`) while the inplace
branch called `qr!(A, alg.pivot)` (returning `QRPivoted`). Routes
sparse-CSC/GPU/cusparse to `qr(A)` explicitly and uses
`qr(A, alg.pivot)` for all other CPU paths so all branches agree
on the return type for a given pivot type.
2. `_default_lu_solve_with_fallback`, `_do_qr_fallback`, `_reuse_qr_fallback`
and the non-LU branches of the `solve!(cache, ::DefaultLinearSolver)`
@generated function read `sol.u`/`sol.resid`/`sol.cache`/`sol.stats`
from an inner `sol`. During precompile, inference of those inner
`solve!(cache, $specific_alg)` calls hits the inference complexity
cap and gets cached with `rettype = Any` — so `sol` is `Any`,
`typeof(sol.u)`/`typeof(sol.resid)`/`typeof(sol.cache)`/`typeof(sol.stats)`
all become abstract type parameters in the constructed LinearSolution.
Fix: build the wrapping LinearSolution from the statically-typed
`cache` (`cache.u`, `nothing` for resid, `cache` itself, `nothing` for
stats). `retcode` and `iters` are fixed-type fields of LinearSolution
(not type parameters), so we keep `sol.retcode`/`sol.iters` to preserve
that runtime info. Also drops unused `args...; kwargs...` forwarding
through the helpers — the inner `solve!` methods ignore those.
Trade-off: `stats` is now `nothing` instead of `sol.stats` when wrapping
through the default solver. For all current LinearSolve factorization
solvers `stats` is already `nothing`, so nothing is lost in practice.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Adds a dedicated test/jet/ group (mirroring the layout Sundials.jl uses
for its JET QA tests) that pins the type-inference contract for
`solve!(cache)` across the default solver and many algorithms.
Layout follows Sundials.jl test/jet/: separate Project.toml with `JET`
under `compat = "0.9, 0.10, 0.11"`, and a self-contained runtests.jl
that the GROUP=JET branch of the top-level runtests.jl activates and
includes.
What the tests pin:
- `Core.Compiler.return_type(solve!, Tuple{cacheT})` for the default
solver must be concrete and a subtype of
`LinearSolution{Float64, 1, Vector{Float64}}`. This is the
user-visible regression these tests guard against.
- `solve!(init(prob, alg))` infers concretely for each of:
LUFactorization, GenericLUFactorization, QRFactorization
(ColumnNorm and NoPivot), DiagonalFactorization, SVDFactorization,
CholeskyFactorization, NormalCholeskyFactorization.
- BunchKaufmanFactorization and LDLtFactorization have unrelated
inference issues; tracked as `@test_broken` so this group ratchets
forward if they're fixed.
- `JET.@test_opt` on the default solver is `broken = true` for now —
the @generated default solver dispatches to every algorithm branch
at inference time and several of those still have unrelated runtime
dispatch sites inside Krylov/LinearAlgebra. The concrete-rettype
checks are the load-bearing test in this group.
Wires GROUP=JET into both test/runtests.jl and .github/workflows/Tests.yml.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
1744b34 to
032a18b
Compare
- Add spaces around `=` in `JET.@test_opt` keyword args (Runic). - Exclude `group: JET, version: pre` in the CI matrix — JET 0.9–0.11 (the versions pinned to match Sundials.jl) don't support Julia 1.13.0-rc1 yet, mirroring the existing NoPre group exclusion. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Contributor
Author
|
Marking ready for review. CI summary: 197 pass, 14 fail, 10 pending. The 14 failures are all pre-existing on
The new |
4 tasks
Drop the separate test/jet/ group introduced earlier in this PR. The
existing NoPre group already has test/nopre/jet.jl with JET tests for
LinearSolve, so the new concrete-rettype checks belong there alongside
the existing @test_opt checks rather than in a parallel group with its
own Project.toml and CI matrix entry.
Moves the contents of test/jet/runtests.jl into the bottom of
test/nopre/jet.jl as two new testsets:
- "solve!(cache) returns concrete LinearSolution — default solver":
`Core.Compiler.return_type(_solve_default, Tuple{Matrix{Float64},
Vector{Float64}})` must be concrete and a subtype of
`LinearSolution{Float64, 1, Vector{Float64}}`.
- "solve!(cache) is concrete for each algorithm": same check for
LUFactorization, GenericLUFactorization, QRFactorization (ColumnNorm
and NoPivot), DiagonalFactorization, SVDFactorization,
CholeskyFactorization, NormalCholeskyFactorization. BunchKaufman and
LDLt remain `@test_broken` for unrelated inference issues.
Removes the test/jet/ directory, the GROUP == "JET" branch in
test/runtests.jl, and the JET entries (matrix value + pre-release
exclude) from .github/workflows/Tests.yml. JET is already a dep of the
NoPre Project.toml, so no Project.toml changes are needed.
Verified locally on Julia 1.12.4: GROUP=NoPre passes with 27 passing /
8 broken in the JET Tests safetestset (the 8 broken match the pre-PR
broken set plus BunchKaufman/LDLt).
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ignore until reviewed by @ChrisRackauckas.
Summary
solve!(cache)returningLinearSolution{_A, _B, _C, _D, DefaultLinearSolver, _E, _F} where {...}(a UnionAll over 6 free type parameters) instead of a concreteLinearSolution.JETtest group (mirroring the layout Sundials.jl uses for its JET QA) that pins the inferred return type ofsolve!(cache)for the default solver and many algorithms.Reproduction (current
main, v3.78.0)After this PR:
Root cause
Two stacked causes:
do_factorization(::QRFactorization, A, b, u)returned aUnion. ForQRFactorization{ColumnNorm}, the non-inplace branch fell through toqr(A)(droppingalg.pivot, returningQRCompactWY) while the inplace branch calledqr!(A, alg.pivot)(returningQRPivoted). Inference gaveUnion{QRCompactWY, QRPivoted}— type-unstable._default_lu_solve_with_fallback/_do_qr_fallback/_reuse_qr_fallbackreadsol.{u,resid,cache,stats}from anAny-typed innersol. During precompile, when inference processes the@generated solve!(cache, ::DefaultLinearSolver), the innersolve!(cache, $specific_alg)calls hit Julia's inference complexity cap and are cached withrettype = Any(you can verify by readingMethod.specializationsfor the cached MethodInstances — every non-LU algorithm showsrettype: Any). The helpers then readsol.u/sol.resid/sol.cache/sol.statsand pass them tobuild_linear_solution, which usestypeof(u)/typeof(resid)/typeof(cache)/typeof(stats)as type parameters — so those four parameters of the constructedLinearSolutionbecome abstract.Fix
src/factorization.jlRoute sparse-CSC / GPU / cusparse to
qr(A)(no pivot — SPQR and CUDA'sqrdon't accept a pivoting strategy), and useqr(A, alg.pivot)for all other CPU paths. This makesdo_factorizationreturn a single concrete type determined byQRFactorization{P}.src/default.jlIn
_default_lu_solve_with_fallback,_do_qr_fallback,_reuse_qr_fallback, and the non-LU branches of the@generated solve!(cache, ::DefaultLinearSolver), build the wrappingLinearSolutionfrom the statically-typedcache:cache.uinstead ofsol.unothinginstead ofsol.residcacheinstead ofsol.cachenothinginstead ofsol.statsretcodeanditersare fixed-type fields ofLinearSolution(ReturnCode.T/Int, not type parameters), so we still takesol.retcodeandsol.itersto preserve that runtime info even whensol::Any.Also drops unused
args...; kwargs...forwarding through the helpers — the innersolve!methods ignore those anyway.Trade-off
statsis nownothinginstead ofsol.statswhen wrapping through the default solver. For all current LinearSolve factorization solversstatsis alreadynothing, so nothing is lost in practice. If a future Krylov method populatedstatsand were selected byDefaultLinearSolver, that field would be dropped — flagging so we can decide if/when that becomes a real issue.New
JETtest groupMirrors the layout Sundials.jl uses for its JET QA tests:
test/jet/Project.toml—JETpinned at0.9, 0.10, 0.11(same as Sundials).test/jet/runtests.jl— self-contained group:Core.Compiler.return_type(solve!, Tuple{cacheT})for the default solver must be concrete and a subtype ofLinearSolution{Float64, 1, Vector{Float64}}.solve!(init(prob, alg))infers concretely forLUFactorization,GenericLUFactorization,QRFactorization{ColumnNorm},QRFactorization{NoPivot},DiagonalFactorization,SVDFactorization,CholeskyFactorization,NormalCholeskyFactorization.BunchKaufmanFactorization/LDLtFactorizationare@test_broken(unrelated inference issues).JET.@test_opton the default solver isbroken = truefor now — the@generatedumbrella dispatches to every algorithm branch at inference time and several of those still have unrelated runtime dispatch sites inside Krylov/LinearAlgebra. The concrete-rettype checks are the load-bearing tests in this group.Wired into both
test/runtests.jl(GROUP=JET) and.github/workflows/Tests.yml.Test plan
GROUP=Core Pkg.test("LinearSolve")passes locally on Julia 1.10.11GROUP=Core2 Pkg.test("LinearSolve")passes locally on Julia 1.10.11GROUP=JET Pkg.test("LinearSolve")passes locally on Julia 1.10.11 (10 passed, 3 broken as documented)@code_typed solve!(cache)returns concrete on Julia 1.10.11 and 1.12.4