Skip to content

Merge jet_kwargs over run_qa's JET defaults instead of replacing them - #52

Merged
ChrisRackauckas merged 4 commits into
mainfrom
agent/jet-kwargs-merge
Aug 15, 2026
Merged

Merge jet_kwargs over run_qa's JET defaults instead of replacing them#52
ChrisRackauckas merged 4 commits into
mainfrom
agent/jet-kwargs-merge

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Aug 15, 2026

Copy link
Copy Markdown
Member

Note

Please ignore this PR until it has been reviewed by @ChrisRackauckas.

What changed and why

run_qa declared jet_kwargs as a whole-NamedTuple default,
(; target_modules = (pkg,), mode = :typo), and splatted it straight into
JET.test_package. Because that is an ordinary default keyword value, any
caller-supplied NamedTuple replaces it wholesale — silently dropping mode = :typo and
reverting JET to its default and far more expensive BasicPass.

This is not hypothetical: a GitHub code search returns 24 hits for target_defined_modules
in the SciML org. DifferentialEquations.jl, twelve OrdinaryDiffEq sublibraries,
NonlinearSolveSciPy, LinearSolvePyAMG and others all pass
jet_kwargs = (; target_defined_modules = true) and are therefore running BasicPass
rather than the typo pass their run_qa call reads as requesting.

The fix merges the caller's entries over the standard configuration, so a partial override
keeps the standard setting for every key it does not mention. target_defined_modules is
JET's deprecated spelling of target_modules, so a caller passing it owns that slot and
suppresses the target_modules default instead of receiving both settings for one slot.

Verification

The new assertions were run against the unfixed src/ first (test file kept, source
stashed), and they fail on exactly the reported behaviour:

run_qa explicit-module path: Test Failed at test/runtests.jl:543
  Expression: (FakeJET.LAST_TEST_KWARGS[]).mode === :typo
   Evaluated: nothing === typo

run_qa explicit-module path: Test Failed at test/runtests.jl:544
  Expression: (FakeJET.LAST_TEST_KWARGS[]).target_modules == (SciMLTesting,)
   Evaluated: nothing == (SciMLTesting,)

Test Summary: | Pass  Fail  Error  Total   Time
SciMLTesting  |  381     3      3    387  53.6s

nothing === typo is the bug itself: a partial override erased the mode. With the fix
applied, the full suite passes:

# Julia 1.12.6
$ julia --project -e 'using Pkg; Pkg.test()'
Test Summary: | Pass  Total     Time
SciMLTesting  |  392    392  1m12.0s
     Testing SciMLTesting tests passed

# Julia 1.10.11 (lts)
$ julia +1.10 --project -e 'using Pkg; Pkg.resolve(); Pkg.test()'
SciMLTesting  |  376    376  43.7s
     Testing SciMLTesting tests passed

Both supported Julia versions pass. The 376-vs-392 difference is the
@static if VERSION >= v"1.11" gated block, not a skipped check.

Also run locally, both clean:

$ julia -e 'using Runic; exit(Runic.main(["--check","--diff","src/SciMLTesting.jl","test/runtests.jl"]))'   # exit 0
$ typos src/ test/                                                                                          # exit 0

Versioning and merge order

This ships as 2.10.0, a minor bump. It is a semantics change to public API: every
existing caller that passes jet_kwargs moves from JET's BasicPass to the intended
TypoPass. That must not go out as a patch.

The number is picked to stack cleanly rather than collide:

version
latest registered in General 2.8.0
main 2.8.0
#51 (Scope the rendered API-docs check) 2.9.0
this PR 2.10.0

This PR therefore assumes #51 lands first. If the order ends up reversed, this one
should be rebased down to 2.9.0 and #51 moved up to 2.10.0 — say the word and I'll
adjust rather than let the two claim the same number.

Why this cannot redden a currently-green lane

The two defaults being filled in can only reduce the number of reports relative to
today's behaviour: target_modules is a filter, and :typo (TypoPass) is strictly
narrower than the :basic (BasicPass) that callers are silently getting now. An explicit
caller entry still wins over the default it collides with, which the new tests assert for
both mode and target_modules. So repos currently green stay green, and repos whose JET
step is failing on BasicPass findings may newly pass.

What I did not verify

  • The repo's Downgrade job and the macOS/Windows/pre legs of the matrix were not run
    locally; CI covers them.
  • There is no docs/make.jl in this repo, so no docs build exists to run. The docstring
    change is covered by the package's own run_api_docs check, which passes in the suite
    above. No public API was added — _standard_jet_kwargs is internal, matching the
    existing _standard_aqua_kwargs / _jet_report_kwargs helpers.

Worth a reviewer's push-back

  • This is a behaviour change for existing callers, even though it is a bug fix. Every
    repo passing jet_kwargs today switches from BasicPass to TypoPass. That is the
    intended configuration, but it is a real change in what those lanes check, and it may be
    worth a minor rather than patch version bump.
  • The target_defined_modules special case encodes a JET-version-specific alias. JET 0.11
    deprecates that config (rewriting it to target_modules = (pkgmod,)) and JET 0.12
    removes it outright, so the branch becomes dead once callers migrate.

Context

Found while investigating why SciML/ModelingToolkit.jl's root QA lane has never been
green. To be explicit: this PR does not fix that lane. MTK's JET step exhausts the CI
runner regardless of report pass — measured at 23.3 GB (:basic) and 28.6 GB (:typo),
neither completing — because JET's target_modules filtering happens in
configured_reports only after the analysis finishes, so neither mode nor
target_modules bounds the abstract interpretation that actually grows. That is a separate
upstream JET scalability issue.

`jet_kwargs` was declared as a whole-NamedTuple default,
`(; target_modules = (pkg,), mode = :typo)`, and splatted straight into
`JET.test_package`. Any caller-supplied NamedTuple therefore replaced it
wholesale, silently dropping `mode = :typo` and reverting JET to its much more
expensive `BasicPass`.

Merge the caller's entries over the standard configuration so a partial override
keeps the standard setting for every key it omits. `target_defined_modules` is
JET's deprecated spelling of `target_modules`, so a caller passing it suppresses
the `target_modules` default rather than receiving both.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
This repo has no docs/make.jl and no documentation workflow; the file was
generated by a local build attempt and swept in by `git add -A`. Upstream tracks
only docs/src/api.md.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Two follow-ups before this is ready to merge.

1. Stray file removed. 7883a01 drops a docs/Project.toml that my local docs-build
attempt generated and that git add -A swept into the first commit. This repo has no
docs/make.jl and no documentation workflow — upstream tracks only docs/src/api.md. The
branch is now exactly the intended change:

 src/SciMLTesting.jl | 27 ++++++++++++++++++++++-----
 test/runtests.jl    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 5 deletions(-)

2. This needs a minor version bump, and it must be sequenced with #51.

This PR deliberately does not touch Project.toml, so as it stands it would ship under
whatever version main carries — currently 2.8.0. That would be wrong: this is a
behaviour change for every existing caller that passes jet_kwargs (they move from
BasicPass to TypoPass, which is the intended configuration but is a real change in what
their QA lane checks), so it warrants a minor bump rather than riding along on a patch.

The exact number depends on merge order — #51 ("Scope the rendered API-docs check to what
each @autodocs block renders") already bumps 2.8.0 → 2.9.0:

Rather than guess and collide, I've left the version alone. Happy to set it once the
ordering is decided.

Local verification (both supported Julia versions, with the fix applied):

Julia 1.12.6 Julia 1.10.11 (lts)
Pkg.test() 392 / 392 pass 376 / 376 pass

The 376-vs-392 difference is the @static if VERSION >= v"1.11" gated block, not a
skipped check. runic --check and typos both exit 0. The new assertions were run against
the unfixed source first and fail on the reported behaviour (Evaluated: nothing === typo),
so they discriminate.

Merging `jet_kwargs` over the defaults instead of replacing them changes
behaviour for every existing caller that passes it: they move from JET's
`BasicPass` to the intended `TypoPass`. That is a semantics change to public API,
so it needs a minor bump rather than riding along on a patch.

2.9.0 is claimed by #51, so this takes the next unreleased minor, 2.10.0, and
stacks behind it.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review August 15, 2026 09:04
@ChrisRackauckas
ChrisRackauckas merged commit c99caa8 into main Aug 15, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants