Merge jet_kwargs over run_qa's JET defaults instead of replacing them - #52
Conversation
`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>
|
Two follow-ups before this is ready to merge. 1. Stray file removed. 2. This needs a minor version bump, and it must be sequenced with #51. This PR deliberately does not touch The exact number depends on merge order — #51 ("Scope the rendered API-docs check to what
Rather than guess and collide, I've left the version alone. Happy to set it once the Local verification (both supported Julia versions, with the fix applied):
The 376-vs-392 difference is the |
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>
Note
Please ignore this PR until it has been reviewed by @ChrisRackauckas.
What changed and why
run_qadeclaredjet_kwargsas a whole-NamedTuple default,(; target_modules = (pkg,), mode = :typo), and splatted it straight intoJET.test_package. Because that is an ordinary default keyword value, anycaller-supplied NamedTuple replaces it wholesale — silently dropping
mode = :typoandreverting JET to its default and far more expensive
BasicPass.This is not hypothetical: a GitHub code search returns 24 hits for
target_defined_modulesin the SciML org. DifferentialEquations.jl, twelve OrdinaryDiffEq sublibraries,
NonlinearSolveSciPy, LinearSolvePyAMG and others all pass
jet_kwargs = (; target_defined_modules = true)and are therefore runningBasicPassrather than the typo pass their
run_qacall 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_modulesisJET's deprecated spelling of
target_modules, so a caller passing it owns that slot andsuppresses the
target_modulesdefault instead of receiving both settings for one slot.Verification
The new assertions were run against the unfixed
src/first (test file kept, sourcestashed), and they fail on exactly the reported behaviour:
nothing === typois the bug itself: a partial override erased the mode. With the fixapplied, the full suite passes:
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:
Versioning and merge order
This ships as
2.10.0, a minor bump. It is a semantics change to public API: everyexisting caller that passes
jet_kwargsmoves from JET'sBasicPassto the intendedTypoPass. That must not go out as a patch.The number is picked to stack cleanly rather than collide:
2.8.0main2.8.02.9.02.10.0This PR therefore assumes #51 lands first. If the order ends up reversed, this one
should be rebased down to
2.9.0and #51 moved up to2.10.0— say the word and I'lladjust 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_modulesis a filter, and:typo(TypoPass) is strictlynarrower than the
:basic(BasicPass) that callers are silently getting now. An explicitcaller entry still wins over the default it collides with, which the new tests assert for
both
modeandtarget_modules. So repos currently green stay green, and repos whose JETstep is failing on
BasicPassfindings may newly pass.What I did not verify
prelegs of the matrix were not runlocally; CI covers them.
docs/make.jlin this repo, so no docs build exists to run. The docstringchange is covered by the package's own
run_api_docscheck, which passes in the suiteabove. No public API was added —
_standard_jet_kwargsis internal, matching theexisting
_standard_aqua_kwargs/_jet_report_kwargshelpers.Worth a reviewer's push-back
repo passing
jet_kwargstoday switches fromBasicPasstoTypoPass. That is theintended configuration, but it is a real change in what those lanes check, and it may be
worth a minor rather than patch version bump.
target_defined_modulesspecial case encodes a JET-version-specific alias. JET 0.11deprecates that config (rewriting it to
target_modules = (pkgmod,)) and JET 0.12removes it outright, so the branch becomes dead once callers migrate.
Context
Found while investigating why SciML/ModelingToolkit.jl's root
QAlane has never beengreen. 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_modulesfiltering happens inconfigured_reportsonly after the analysis finishes, so neithermodenortarget_modulesbounds the abstract interpretation that actually grows. That is a separateupstream JET scalability issue.