Skip to content

Use DEFAULT_CALLBACK const in OptimJL/Optimisers __init - #1407

Draft
ChrisRackauckas-Claude wants to merge 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:opt-default-callback-const
Draft

ChrisRackauckas-Claude wants to merge 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:opt-default-callback-const

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Summary

OptimizationOptimJL and OptimizationOptimisers defaulted callback to a fresh anonymous (args...) -> (false) in __init. When the call goes through Julia's keyword sorter (any kwargs), that default is evaluated again and gets a different type, so typeof(init(prob, LBFGS())) != typeof(init(prob, LBFGS(); maxiters=100)) and a second solve that adds a kwarg recompiles the whole chain. Both packages already using OptimizationBase, which exports the public DEFAULT_CALLBACK (NullCallback, (args...) -> false); point the defaults at that const instead (same pattern as MadNLP/ODE/PRIMA).

Measurements (amdci2, process CPU via /proc/self/stat utime+stime, OPENBLAS_NUM_THREADS=1, JULIA_NUM_THREADS=1, taskset -c 2, 5 alternating master/PR fresh processes)

OptimizationOptimJL Rosenbrock + Optim.LBFGS + ForwardDiff:

master (n=5) PR (n=5)
TTFS LBFGS (first solve, no kwargs) median 6.36 s (range 6.13–7.10) median 6.38 s (range 6.32–7.68)
2nd solve with maxiters=100 median 0.95 s (0.88–0.96) median 0.14 s (0.13–0.14)

First-solve: no clear regression beyond noise on remeasure (medians 6.36 vs 6.38; one noisy PR outlier at 7.68 and one master at 7.10). The auditor's earlier 5.91→6.06–6.20 bump did not reproduce as a stable effect here. Second-solve with kwargs: clear win (~0.95 → ~0.14 s CPU).

OptimizationOptimisers: regression test confirms the same init type mismatch on master; solve always requires maxiters/epochs, so the no-kwargs→kwargs TTFS pattern is OptimJL-specific. Fix kept for init type stability + consistency.

Other lib/Optimization*/src sites still using anonymous (args...) -> (false) (Sophia, SciPy, NLopt, Metaheuristics): left alone — same pattern, not TTFS-measured in this PR.

Regression test

typeof(init(...)) == typeof(init(...; maxiters=100)) in each sublib's test/core_tests.jl.

  • Before (src stashed): FAIL — distinct var"#6#7" vs var"#8#9" callback types (before.txt)
  • After: PASS (after.txt); also PASS on Julia 1.10.12

Test-group tails

  • OptimizationOptimJL Core: Core | 6970 Pass; tests passed
  • OptimizationOptimisers Core: Core | 49 Pass, 1 Broken; tests passed
  • OptimizationOptimJL QA: Quality Assurance | 54 Pass, 1 Broken; tests passed
  • OptimizationOptimisers QA: Quality Assurance | 80 Pass, 1 Broken; tests passed
  • Root GROUP=QA (via test/qa): Quality Assurance | 21 Pass

Not verified

  • TTFS for Sophia/SciPy/NLopt/Metaheuristics (same anonymous default still present)
  • Runtime @benchmark (this is a compile-time / type-stability fix; numbers are process CPU)
  • Full root Pkg.test extras env (Enzyme/GPU) — used dedicated test/qa project for GROUP=QA
  • Downstream packages

Reviewer pushback

  • Is a small first-solve TTFS noise band worth worrying about? Remeasure says no stable regression.
  • Should the remaining anonymous-default sites be fixed in a follow-up for consistency even without TTFS numbers?
  • Optimisers always needs maxiters/epochs for solve — is the Optimisers half of the diff justified on init type stability alone?

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

Independent review: pending

🤖 Generated with Cursor Agent 2026.10.01-14929f9 (model: unknown (Cursor auto)), transcript /home/crackauc/sandbox/goals/performance/jobs/opt/opt-defcb/log.txt on amdci2; orchestrated by Claude Code (claude-opus-5-5[1m]) https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Made with Cursor

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #1407 (comment)

Anonymous `(args...) -> (false)` defaults get a fresh type when kwargs
trigger the kw sorter, so a second solve with maxiters recompiles the
whole chain. Point at the shared NullCallback const instead.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Cursor Agent <noreply@cursor.com>
Agent-Harness: Cursor Agent 2026.10.01-14929f9
Agent-Model: unknown (Cursor auto)
Agent-Session: local session, transcript /home/crackauc/sandbox/goals/performance/jobs/opt/opt-defcb/log.txt on amdci2
@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): CHANGES, risk low.

Full review

VERDICT: CHANGES
RISK: low

The change is correct, and it does what the PR body says. On master, the anonymous (args...) -> (false) default gets a different closure type when the call goes through the kw-sorter. Swapping it for DEFAULT_CALLBACK (NullCallback()) makes init produce the same cache type with or without kwargs, and it changes no results. One thing needs a one-token fix before merge: the bare DEFAULT_CALLBACK name semantically conflicts with two open sibling PRs.

Blocking findings

  1. The bare DEFAULT_CALLBACK breaks every default-callback init/solve once the open explicit-imports PRs land. Locations: lib/OptimizationOptimJL/src/OptimizationOptimJL.jl:178 and lib/OptimizationOptimisers/src/OptimizationOptimisers.jl:47.
    • Both packages see the name only through the blanket using OptimizationBase. Two open PRs replace that line with using OptimizationBase: OptimizationBase, OptimizationCache: OptimizationOptimJL: make implicit imports explicit, un-break no_implicit_imports QA check #1392 for OptimJL and QA: make OptimizationOptimisers imports explicit #1397 for Optimisers.
    • Git merges these with no textual conflict, because the edited lines are about 150 lines apart. Each PR's CI stays green on its own.
    • Confirmed by running: I took git archive HEAD lib, then ran git apply with the src/ hunks of both PRs' diffs. It applied cleanly. I then ran scratch/probe.jl against that tree, and the first solve(prob, LBFGS()) failed with UndefVarError: DEFAULT_CALLBACK not defined in OptimizationOptimJL (OptimizationOptimJL.jl:173).
    • Whichever order these merge in, master ends up with the default callback broken unless someone notices.
    • Required change: write callback = OptimizationBase.DEFAULT_CALLBACK in both places. That is the form already used in OptimizationPRIMA.jl:60 and OptimizationODE.jl:102,113. It works with both the blanket and the explicit using. It also adds no new implicit import to modules whose no_implicit_imports QA check is currently marked broken and being fixed. DEFAULT_CALLBACK is exported from OptimizationBase, so the qualified access passes all_qualified_accesses_are_public. An alternative is to add DEFAULT_CALLBACK to the explicit import lists, but that only helps if OptimizationOptimJL: make implicit imports explicit, un-break no_implicit_imports QA check #1392 and QA: make OptimizationOptimisers imports explicit #1397 are updated too.

Non-blocking findings

  • Fail-before / pass-after. Confirmed by running: on the master sources the probe reports OptimJL init types equal: false (cb types var"#6#7" / var"#8#9") and Optimisers init types equal: false (var"#4#5" / var"#6#7"). On the PR it reports true for both, with NullCallback on each side. The new @test typeof(init(...)) == typeof(init(...; maxiters = 100)) therefore fails on master and passes on the PR. It is a real assertion that can fail, not one derived from the code's own output.
  • Same results on PR and master. Confirmed by running:
    • The LBFGS solution is bit-identical: [1.0000000001101883, 1.000000000213478], Success, with and without maxiters.
    • NelderMead, BFGS and Fminbox(LBFGS) give identical u. The cache types now also match for these with/without kwargs.
    • SAMIN u differs between runs because SAMIN is stochastic and the RNG is not seeded. Retcode is MaxIters in both.
    • A user callback that halts after 3 calls still halts after 3 calls.
    • A callback returning a non-Bool still raises the "callback should return a boolean" error.
    • The Adam solve and an Adam user callback (5 calls) behave the same as on master.
  • Speedup. Confirmed by running, but it is one wall-clock sample per side on this laptop, not CPU-time medians. The second solve with maxiters = 100 took 0.526 s on master and 0.059 s on the PR. The first solve was 3.12 s on master and 3.10 s on the PR. Direction and magnitude match the PR body's ~0.95 → ~0.14 s CPU claim. I did not reproduce the amdci2 /proc/self/stat numbers.
  • Test comment. The added comment line ("Default callback must be a stable const so …") explains why, rather than narrating the change. The ratio of comment lines (2 of 20 added) is acceptable.
  • PR body. The body links no issue (closingIssuesReferences is empty), so there is no parent tracking issue to check interface fit against. The remaining anonymous defaults in Sophia, SciPy, NLopt and Metaheuristics are left out on purpose and are listed as such. A follow-up for those should use the qualified OptimizationBase.DEFAULT_CALLBACK form for the same reason as blocking finding 1.
  • CI (https://github.com/SciML/Optimization.jl/pull/1407/checks): 57 pass and 2 fail. The failures are "ModelingToolkit.jl/Optimization Downstream Tests" (30 s) and "downgrade-sublibraries (lib/OptimizationReactant)". This PR does not touch Reactant, and the MTK job died in 30 s, so both look unrelated, but I did not confirm they are also red on master. The "Julia pre" Core jobs for both sublibs pass in 34 s and 16 s, so they are probably not exercising the tests. That is pre-existing and not caused by this PR.
  • Compat. OptimizationBase = "5.5.0" is fine. Confirmed: at the 5.5.0 release commit 7eb8ca2, NullCallback and const DEFAULT_CALLBACK are defined and DEFAULT_CALLBACK is exported.

What I ran

All runs used TMPDIR set to the review tmp/ directory, Julia 1.11 (the default channel), on macOS M2.

  • git diff origin/master...HEAD: 4 files changed, +20/−2. This matches the PR description. The repo has no AGENTS.md or CLAUDE.md at its root.
  • lib/OptimizationOptimJL, OPTIMIZATION_TEST_GROUP=Core julia --project -e 'using Pkg; Pkg.test()': Core | 6970 Pass, tests passed. This matches the PR body.
  • lib/OptimizationOptimisers, same command: Core | 49 Pass 1 Broken, tests passed. The Broken is the pre-existing @test_broken; the diff adds none.
  • OPTIMIZATION_TEST_GROUP=QA for both sublibs: Quality Assurance | 54 Pass 1 Broken for OptimJL and 80 Pass 1 Broken for Optimisers, both passed. The Broken is the pre-existing ei_broken = (:no_implicit_imports,). This matches the PR body.
  • scratch/probe.jl, run in fresh processes against the PR tree, against git archive origin/master, and against the PR plus the src hunks of OptimizationOptimJL: make implicit imports explicit, un-break no_implicit_imports QA check #1392 and QA: make OptimizationOptimisers imports explicit #1397. It checks init type equality for LBFGS, NelderMead, BFGS, Fminbox, SAMIN and Adam, compares solutions, checks user-callback halting and the non-Bool callback error, and times the first and second solves. Logs: scratch/probe_pr.log and scratch/probe_master.log; the combined-tree run is the UndefVarError above.
  • julia +1.12 --project=@runic -m Runic --check --diff on the four changed files: clean.
  • git diff origin/master...HEAD | typos -: clean.
  • gh pr checks 1407, plus gh pr diff 1392 and gh pr diff 1397.
  • All Julia processes I started have exited.

What I did not verify

  • The amdci2 CPU-time medians and the first-solve "no regression" claim. I have one wall-clock sample per side, which is consistent with no regression but is not a measurement.
  • Julia 1.10 (the PR body claims a pass on 1.10.12). I did not run it.
  • Whether the two red CI jobs (MTK downstream, OptimizationReactant downgrade) also fail on master.
  • Downstream packages such as DiffEqFlux and SciMLSensitivity that call OptimizationOptimisers. I can't see how they'd be affected: NullCallback is callable with any arguments and returns false, exactly like the closure it replaces, and nothing in lib/ dispatches on the old closure type.

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

Bare DEFAULT_CALLBACK breaks once SciML#1392/SciML#1397 replace blanket
`using OptimizationBase` with an explicit import list. Match PRIMA/ODE.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Cursor Agent <noreply@cursor.com>
Agent-Harness: Cursor Agent 2026.10.01-14929f9
Agent-Model: unknown (Cursor auto)
Agent-Session: local session, transcript /home/crackauc/sandbox/goals/performance/jobs/opt/opt-defcb/log.txt on amdci2
@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

PR: #1407 (no linked issue; closingIssuesReferences is empty). The diff changes two lines of source: the callback kwarg default in SciMLBase.__init goes from a fresh (args...) -> (false) closure to the exported, documented OptimizationBase.DEFAULT_CALLBACK (NullCallback()). It also adds one regression @testset per sublib.

Blocking findings

None.

Non-blocking findings

  1. The regression test checks a broader property than the fix (lib/OptimizationOptimJL/test/core_tests.jl:434-440, lib/OptimizationOptimisers/test/core_tests.jl:332-339). From reading: the test asserts typeof(init(prob, alg)) == typeof(init(prob, alg; maxiters = 100)) across the whole OptimizationCache type. Today this does isolate the callback type. I confirmed by running that the two cache types differ only through the callback on master and are equal on the PR. But if maxiters (or anything else that depends on kwargs) ever ends up in the cache's type parameters, this test will fail for a reason unrelated to the callback. Adding @test cache.callback === OptimizationBase.DEFAULT_CALLBACK would pin the exact contract. This is optional; the current assertion does fail before the fix and pass after it (see What I ran).
  2. The PR body's "left alone" list is out of date. The four sites it says remain (Sophia, SciPy, NLopt, Metaheuristics) are now handled by sibling PR Use DEFAULT_CALLBACK const in SciPy/Sophia/NLopt/Metaheuristics __init #1412, which uses the same OptimizationBase.DEFAULT_CALLBACK spelling and the same testset name. The two PRs touch disjoint files, so they don't conflict. The qualified OptimizationBase.DEFAULT_CALLBACK (commit 6a8e6e6) also works with the explicit-import PRs OptimizationOptimJL: make implicit imports explicit, un-break no_implicit_imports QA check #1392 and QA: make OptimizationOptimisers imports explicit #1397. Both rewrite the import to using OptimizationBase: OptimizationBase, OptimizationCache, so the module name stays bound (checked with gh pr diff). OptimizationOptimJL: default extended_trace to false #1406 edits the same __init in OptimJL but not the callback line.
  3. Attribution footer gives the model as unknown (Cursor auto). Chris's CLAUDE.md asks for the exact model ID. This is a process nit and doesn't affect the code.
  4. The red CI check is a resolver error, not this change. CI check ModelingToolkit.jl/Optimization / Downstream Tests is red. Its job log (https://github.com/SciML/Optimization.jl/actions/runs/38090487366/job/114325725681) stops at Unsatisfiable requirements detected for package TimerOutputs before any test runs. So the failure comes from the downstream environment and not from this change. I didn't check whether master shows the same failure.

What I ran

All runs were local on macOS, Julia 1.11 (default juliaup channel), with TMPDIR set to the review tmp dir.

  • git diff origin/master...HEAD: 4 files, +20/−2, matching the PR body. git diff --check was clean. typos on both test files was clean. julia +1.12 --project=@runic -m Runic --check --diff on all 4 changed files exited 0.
  • Before/after probe (scratch/probe.jl). I ran it in two scratch envs that Pkg.develop either the PR tree or a copy whose lib/OptimizationOptimJL/src and lib/OptimizationOptimisers/src were reset to origin/master. Confirmed by running:
    • master: OptimJL callback types var"#6#7" vs var"#8#9", cache types equal = false. Optimisers callback types var"#4#5" vs var"#6#7", cache types equal = false. This reproduces the bug as the PR describes it.
    • PR: both NullCallback | NullCallback, cache types equal = true in both sublibs.
    • Behaviour is unchanged between master and PR, with bit-identical results for:
      • solve(prob, LBFGS()), which gives Success at [1.0000000001101883, 1.000000000213478]
      • a user callback that halts after 3 calls (3 calls, retcode Failure, in both trees)
      • Adam with maxiters = 2000
      • a user callback halting Adam after 5 iterations (5 calls, 5 iterations)
      • Fminbox(LBFGS()) with bounds
      • SAMIN
      • NelderMead without AD
    • In one process, after the first solve, a second solve(prob, LBFGS(); maxiters = 100) took 0.389 s on master and 0.032 s on the PR (wall-clock @elapsed, one sample each). That matches the direction of the PR's ~0.95 → 0.14 s CPU claim, but it is not a reproduction of their measurement protocol.
  • Test suites on the PR head:
    • OPTIMIZATION_TEST_GROUP=Core Pkg.test() in lib/OptimizationOptimJL gave Core | 6970 Pass / 6970 (log scratch/optimjl_core.log). This matches the PR body.
    • The same in lib/OptimizationOptimisers gave Core | 49 Pass, 1 Broken / 50 (log scratch/optimisers_core.log). The single Broken is the existing @test_broken at core_tests.jl:101 on master; the diff adds no @test_broken.
    • git status stayed clean afterwards.
  • Code reading:
    • NullCallback is defined at lib/OptimizationBase/src/OptimizationBase.jl:69-78. It returns false for any arguments, so it is semantically identical to the old closure.
    • DEFAULT_CALLBACK is exported and has a docstring, so this is public API.
    • Nothing in OptimJL or Optimisers branches on the callback's type.
    • The solve.jl:126 NullCallback check only applies to algorithms where !allowscallback, and both sublibs return allowscallback = true.
    • No new dependency, no compat change, no non-public API use. The added comment lines (one per testset) explain why the test exists rather than narrating the change.
  • All Julia processes I started have exited (pgrep found none for this review).

What I did not verify

  • The PR's TTFS table (amdci2, /proc/self/stat CPU, 5 alternating fresh processes). I didn't rerun it; my single wall-clock sample only confirms the direction for the second solve. I also didn't measure the first-solve-with-no-kwargs time.
  • The QA groups for either sublib, root GROUP=QA, and Julia 1.10. I didn't run any of them, so the PR's QA tails and its "also PASS on Julia 1.10.12" claim are unverified here.
  • Downstream packages (MTK, NeuralPDE, DiffEqFlux). Their CI was still running or failed in resolution, as noted above.
  • Whether master's ModelingToolkit downstream job hits the same TimerOutputs resolver failure.

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

This branch has not been deployed

No deployments
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