Fix collect_vars! after late invalidation on Julia 1.10 - #4852
Conversation
Route recursive metadata collection through a dynamic dispatch barrier so late method additions cannot corrupt Julia 1.10 inference while downstream collect_vars! extensions remain visible. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
|
Investigation scratchpad / execution record:
|
|
Clean-master QA classification is now fully local and status-checked: exact |
|
CI classification: |
|
CI classification: |
|
Additional CI classification:
|
|
Completed-cluster confirmation from the actual logs:
None of these failures mention or exercise the new collector barrier/regression. #4849 handles the ambiguity class; #4670 retains the piracy ownership work. |
|
Distributed-cluster investigation update: exact clean |
|
Direct CI confirmation: |
|
The direct |
|
Downstream I reproduced the failure on the exact clean ModelingToolkit base A first-parent compatibility bisect places the stale edge at SBMLToolkit commit I then reran the exact workflow on clean ModelingToolkit So this downstream check should clear once SciML/SBMLToolkit.jl#223 is merged/released; no collector change is implicated. |
|
The Extended/LTS Controls on the same clean master / Julia 1.10 setup:
The first bad RGF source commit is |
|
The newly terminal Exact #4852 downstream job reaches |
|
Catalyst's stale expected-broken follow-up has now merged as Catalyst #1512, and I reran the complete downstream group locally against this collector branch. Exact validation:
Terminal result: exit 0, The complete official group ran for 1829.82 seconds and also cleared the former prerequisites, including Symbolic Stoichiometry 30/30 and DSL Options 317 pass / 1 existing broken. This confirms the recorded |
|
The I reproduced it with exact clean ModelingToolkit base This is a PDEBase behavioral regression, not a missing Symbolics On the same downstream manifest, changing only PDEBase to current master with the 0.1.32 bump makes the official group exit 0 ( |
|
Terminal cluster correction and exact count:
fail with the same One correction to the earlier shorthand: the SciMLBase/SymbolicIndexingInterface job reaches tests and fails at |
|
MethodOfLines cluster is now fully validated:
Thus both #4852 MethodOfLines failures are independent of the collector diff and have direct local plus hosted coverage. |
|
The independent ModelingToolkitBase BVP downgrade failure is now covered by focused draft #4853. Exact clean-master control ( The root cause is the pre-1.17.3 MIRK solution-evaluation buffer, fixed upstream by SciML/BoundaryValueDiffEq.jl#568 and released in With that one-line floor change, the exact downgrade workflow selects MIRK 1.17.3 and the full official |
|
The ModelOrderReduction downstream failure is isolated and fixed in draft PR SciML/ModelOrderReduction.jl#169. Root cause: PDEBase 0.1.31 correctly stopped creating an incidental I reproduced the original |
Consolidated terminal CI ledgerThe hosted matrix is terminal at 64 success / 26 failure / 3 skipped. The 26 failures are fully accounted for; none points to the collector invalidation barrier in this PR.
So the answer to “are all independent follow-ups merged?” is no. The Catalyst/PDEBase/MethodOfLines source and registration pieces are merged; the focused RGF, MTK QA/downgrade, MOR, SBML, StructuralIdentifiability, OrdinaryDiffEq, General compat, and related release steps remain open as listed above. |
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
If the goal is to make it dynamic dispatch, why not just make the wrapper method use @invokelatest instead of this approach with the global Ref?
Replaces the global Ref{Function} cell with @invokelatest, per review.
Both break the Julia 1.10 inference cycle identically and cost the same,
but @invokelatest also resolves in the latest world age, so a downstream
collect_vars! method defined while a collection is already running is
still found. The four-argument fallback returns nothing, so missing such
a method dropped parameters silently rather than erroring.
The @noinline and Base.@constprop :none annotations are no longer needed,
since @invokelatest is already opaque to inference.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
ChrisRackauckas-Claude
left a comment
ChrisRackauckas-Claude
left a comment
There was a problem hiding this comment.
Good call — switched to @invokelatest in 04b6dcd. It isn't only cleaner; it's actually more correct here, and I have measurements for both claims.
Performance: no difference
Julia 1.10.11, three interleaved reps of 20 000 samples each, fresh process per rep, clean master d3917493 as the baseline:
| variant | default case |
bounds case |
|---|---|---|
| master (no barrier) | 4032 B / 85 allocs | 2872 B / 71 allocs |
Ref{Function} |
4064 B / 87 allocs | 2968 B / 77 allocs |
@invokelatest |
4064 B / 87 allocs | 2968 B / 77 allocs |
Allocations are byte-identical between the two barriers. Times are indistinguishable — min over reps 11.57/11.72 µs (default) and 10.93/10.94 µs (bounds) for Ref/@invokelatest, against master's own 11.64 µs and 10.87 µs. The run-to-run spread on both exceeds the gap between them. So there was never a perf argument for the Ref.
Correctness: @invokelatest is strictly better
The Ref[] load is an ordinary dynamic dispatch, so it resolves in the caller's world age. @invokelatest resolves in the latest one. That matters when a downstream collect_vars! method is defined while a collection is already running — e.g. an @eval inside a component or macro that then builds a system in the same top-level call:
Ref{Function} → same-world-age call : [] SILENTLY DROPPED
@invokelatest → same-world-age call : [lp] collected
The silent part is what makes this worth caring about: the four-argument collect_vars! has a catch-all fallback returning nothing, so a method the call can't see doesn't MethodError — it just loses the parameter. That is exactly the failure mode this PR exists to fix, so the Ref version left a variant of the bug reachable. I added a regression test for it (collect_vars! dispatches to same-world-age downstream methods) and confirmed it fails against the Ref implementation and passes with @invokelatest.
I also dropped @noinline and Base.@constprop :none: @invokelatest is already opaque to inference, and the invalidation regression still passes 6/6 without them. That reduces the whole barrier to:
function _call_collect_vars!(unknowns, parameters, expr, iv)
return @invokelatest collect_vars!(unknowns, parameters, expr, iv; depth = 0)
endNo global mutable state and no compiler-annotation scaffolding to explain.
Verification
- Julia 1.10.11: invalidation regression 6/6, new world-age regression 2/2. Clean master
d3917493fails the invalidation regression 2 passed / 4 failed, so the reproduction is real. - Julia 1.12.6: both 6/6 and 2/2; world-age case collected.
- Official
GROUP=InterfaceIon Julia 1.10: 1497 passed, 5 existing broken, 3 failed. All three failures are the samesymbolic_events.jlcallback-accuracy assertion (lines 729/846/866,2.635e-4against a1e-4threshold). An exact clean-masterd3917493control in the identical environment fails at the same three lines with the same values, so they are pre-existing and unrelated to the collector. - Runic 1.7.0 whole-repository
--check .: exit 0.git diff --check: clean.
No tests were skipped, disabled, silenced, or loosened.
One unrelated note for anyone reproducing locally on Julia 1.10: Pkg.test() on ModelingToolkitBase currently dies during resolution with BracketingNonlinearSolve depends on ChainRulesCore, but no such entry exists in the manifest. ChainRulesCore is a weakdep of BracketingNonlinearSolve (the multi-trigger BracketingNonlinearSolveChainRulesCoreExt), correctly recorded in General's WeakDeps.toml, but Pkg 1.10 writes it into the manifest's hard-dep table without installing it. It reproduces on clean master and on Julia 1.12's Pkg.develop, and it is fixed by adding ChainRulesCore to the environment explicitly. Purely a local tooling issue, no bearing on this PR.
3 participants
Please ignore this PR until reviewed by @ChrisRackauckas.
Summary
collect_vars!calls from metadata collection through a non-inlinedRef{Function}dispatch barriercollect_vars!after package load are still dispatched dynamicallycollect_vars!methodWhy
Loading DynamicQuantities and then unrelated packages can invalidate the mutually recursive
collect_vars!/collect_var!inference cycle on Julia 1.10. After that invalidation, parameters used only in defaults can disappear. In downstream initialization this manifested as a missingInitial(x0)parameter and asy0no longer being recognized as an unknown.The first bad ModelingToolkit commit is
22000aa03d317353500c3c73ca01744d069530be(refactor: change collect_vars! for type-stability); its parentca6eb0a92is good. A package-load warmup is not durable because later method additions can invalidate the warmed specialization again.The barrier is deliberately restricted to the recursive four-argument calls inside
collect_var!. The explicitdepth = 0preserves their previous default-keyword semantics, while the abstract function cell prevents inference from rebuilding the problematic mutual cycle. It also avoids reaching into dependency internals or changing public API.Performance
Fresh-process construction microbenchmarks, baseline -> patch:
This is system-construction work, not the nonlinear
solve!path. The warmed ImplicitDiscreteSolve homotopy solve remains 0 B per solve in the separate cache benchmark.Local verification
dq_units.jl: initialization 1/1; invalidation regression 6/6dq_units.jl: initialization 1/1; invalidation regression 6/6[json_p]is collected both before and after the late method addition; late custom dispatch collects[custom_p]GROUP=InterfaceI Pkg.test("ModelingToolkitBase"): 1498 passed, 5 existing broken, 0 failed/erroredGROUP=Initialization: the two collector errors are gone; 583 passed, 9 unrelated DelayDiffEqDEOptionserrors, 14 existing broken. The nine resolver errors are tracked by Retroactively cap DelayDiffEq at OrdinaryDiffEqCore 4.8.0 JuliaRegistries/General#162781.GROUP=QA: JET 54/54 passed. Aqua reports 45 ambiguities and 14 piracy methods; an exact clean-masterd39174937run reproduces the same Aqua 9 passed / 2 failed result, so that baseline failure is being handled separately rather than hidden here.--check .: exit 0git diff --check: cleanNo tests were skipped, disabled, silenced, or loosened.
Investigation/process notes
The work started from the downstream LinearSolve/ModelingToolkit initialization failures discovered while validating the ImplicitDiscreteSolve homotopy cache path. I reproduced the failure with frozen dependencies on Julia 1.10, separated the two collector errors from nine independent DelayDiffEq resolver errors, bisected the collector regression, minimized the invalidation trigger, compared direct calls,
invoke, function wrappers, and a dynamic function cell, verified late downstream dispatch, benchmarked construction overhead, then ran the package groups and formatting checks above. The unrelated clean-master QA failures are being bisected in a separate worktree so this remains one focused fix.