Skip to content

OptimizationMOI: make implicit imports explicit - #1401

Draft
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:qa/optimizationmoi-explicit-imports
Draft

ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:qa/optimizationmoi-explicit-imports

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

OptimizationMOI now imports MathOptInterface, OptimizationBase, SciMLBase, SciMLStructures, SparseArrays, SymbolicIndexingInterface, Symbolics, and LinearAlgebra by name (using X: a, b), including the symbols the implementation uses from each owner. The unused using Reexport was dropped. The QA suite no longer marks no_implicit_imports as broken. reinit! is kept as an explicit SciMLBase import (with a stale-ignore entry) so OptimizationMOI.reinit! continues to resolve for callers and tests; MOI, MOIOptimizationCache, and MOIOptimizationNLPCache remain available.

Verification

Commands ran from lib/OptimizationMOI, with ulimit -n 65536 and the Julia depot / TMPDIR isolated under this job directory.

  • Baseline QA (before changes):

    export TMPDIR=/home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/tmp
    export JULIA_DEPOT_PATH=/home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/scratch/julia-depot
    ulimit -n 65536
    OPTIMIZATION_TEST_GROUP=QA julia --project -e 'using Pkg; Pkg.test()'

    Output tail: Quality Assurance | 20 1 21 2m32.3s; Testing OptimizationMOI tests passed.

  • QA after change (same commands): output tail: Quality Assurance | 21 21 2m34.6s; Testing OptimizationMOI tests passed. The previously broken no_implicit_imports check now runs and passes.

  • Core:

    export TMPDIR=/home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/tmp
    export JULIA_DEPOT_PATH=/home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/scratch/julia-depot
    ulimit -n 65536
    OPTIMIZATION_TEST_GROUP=Core julia --project -e 'using Pkg; Pkg.test()'

    Output tail: Core | 40 2 42 4m49.5s; Testing OptimizationMOI tests passed (the two broken cases are pre-existing @test_broken reinit cases).

  • Runic: julia --project=@runic -e 'using Runic; exit(Runic.main(ARGS))' -- --inplace lib/OptimizationMOI/src/OptimizationMOI.jl lib/OptimizationMOI/test/qa/qa.jl — exit 0.

  • typos: typos lib/OptimizationMOI/src/OptimizationMOI.jl lib/OptimizationMOI/test/qa/qa.jl — exit 0.

  • Scanned lib/, test/, and docs/ for qualified OptimizationMOI.x uses. Kept: MOI (const alias), MOIOptimizationCache, MOIOptimizationNLPCache, and reinit!.

Not verified: CI, the full Optimization.jl monorepo suite, and downstream packages.

Follow-ups (not in this PR)

Left untouched on purpose (same as the existing QA ignore lists):

  • @sync via-owners access: Threads.@sync is reported as accessing Base.@sync via Base.Threads; still ignored under all_qualified_accesses_via_owners / all_qualified_accesses_are_public. Fix later by importing @sync from Base (or equivalent) without changing call sites in this sweep.
  • Ignored non-public qualified accesses (upstream public declarations belong in MathOptInterface / OptimizationBase / SciMLBase / SciMLStructures / SymbolicUtils, not here): ALMOST_DUAL_INFEASIBLE, ALMOST_INFEASIBLE, ALMOST_LOCALLY_SOLVED, ALMOST_OPTIMAL, AbstractNLPEvaluator, AbstractOptimizationCache, AbstractOptimizer, BarrierIterations, CachingOptimizer, Code, DEFAULT_VERBOSE, DUAL_INFEASIBLE, EqualTo, GetAttributeNotAllowed, GreaterThan, INFEASIBLE, INFEASIBLE_OR_UNBOUNDED, INTERRUPTED, INVALID_MODEL, INVALID_OPTION, ITERATION_LIMIT, Integer, LOCALLY_INFEASIBLE, LOCALLY_SOLVED, LessThan, MAX_SENSE, MEMORY_LIMIT, MIN_SENSE, NLPBlock, NLPBlockData, NLPBoundsPair, NODE_LIMIT, NORM_LIMIT, NUMERICAL_ERROR, OBJECTIVE_LIMIT, OPTIMAL, OPTIMIZE_NOT_CALLED, OTHER_ERROR, OTHER_LIMIT, ObjectiveFunction, ObjectiveSense, ObjectiveValue, OptimizationState, OptimizationStats, OptimizerWithAttributes, RawOptimizerAttribute, ReInitCache, ResultCount, SLOW_PROGRESS, SOLUTION_LIMIT, ScalarAffineFunction, ScalarAffineTerm, ScalarQuadraticFunction, ScalarQuadraticTerm, Silent, SolveTimeSec, TIME_LIMIT, TerminationStatus, TerminationStatusCode, TimeLimitSec, Tunable, UniversalFallback, Utilities, VariableIndex, VariablePrimal, VariablePrimalStart, ZeroOne, __init, __solve, _check_and_convert_maxiters, _check_and_convert_maxtime, _process_verbose_param, add_constraint, add_variables, allowscallback, canonicalize, constraint_expr, empty!, eval_constraint, eval_constraint_jacobian, eval_constraint_jacobian_product, eval_constraint_jacobian_transpose_product, eval_hessian_lagrangian, eval_objective, eval_objective_gradient, features_available, get, get_observed, get_p, get_paramsyms, get_syms, hessian_lagrangian_structure, initialize, instantiate, instantiate_function, is_empty, jacobian_structure, objective_expr, optimize!, requiresconshess, requiresconsjac, requiresgradient, requireshessian, set, supports, supports_incremental_interface, supports_opt_cache_interface, supports_sense.

Please ignore this draft until reviewed by @ChrisRackauckas.

Risk assessment

  • Risk: low
  • Blast radius: imports and QA status for OptimizationMOI only. Qualified uses in-repo (OptimizationMOI.MOI, cache types, OptimizationMOI.reinit!) still resolve. Names that were only incidentally reachable through old blanket using (e.g. other SciMLBase exports) no longer are; nothing in lib/, test/, or docs/ uses them, but downstream code spelling them that way would break.
  • Evidence: QA went from 20 pass / 1 broken to 21/21; Core 40 pass / 2 pre-existing broken; Runic and typos exit 0.
  • Independent review: pending
  • Merge: needs review

Cursor Agent CLI 2026.10.01-14929f9 (model: auto); transcript: /home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/log.txt

Made with Cursor

Follow the OptimizationNLopt pattern (SciML#1398): import each used name from its
owner module and drop the no_implicit_imports broken marker. Keep reinit! so
OptimizationMOI.reinit! continues to resolve for callers and tests.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Cursor Agent <noreply@cursor.com>
Agent-Harness: Cursor Agent CLI 2026.10.01-14929f9
Agent-Model: auto
Agent-Session: local session, transcript at /home/crackauc/sandbox/goals/qa-hygiene/jobs/moi-imports/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 medium.

Full review

VERDICT: CHANGES
RISK: medium

PR: #1401 — "OptimizationMOI: make implicit imports explicit" (head 1a26377). It doesn't link an issue or a tracking issue.

Blocking findings

  1. OptimizationMOI.solve, .solve! and .init stop resolving, and code downstream uses them. The PR's risk section says it checked for this, but it only searched this repo.
    lib/OptimizationMOI/src/OptimizationMOI.jl:7-8. On master, using SciMLBase makes every SciMLBase export reachable as OptimizationMOI.<name>. The PR keeps only OptimizationFunction, OptimizationProblem, ReturnCode, remake, reinit!. It keeps reinit! explicitly "so OptimizationMOI.reinit! keeps resolving for callers", but drops the common-interface entry points that callers actually use.
    • Confirmed by running: I built two scratch envs that are identical except for the OptimizationMOI source (master via git archive origin/master, and the PR head). Then I ran import OptimizationMOI; isdefined(OptimizationMOI, n) in each:
      name             master  PR
      solve            true    false
      solve!           true    false
      init             true    false
      AutoForwardDiff  true    false
      AutoSymbolics    true    false
      MinSense/MaxSense true   false
      reinit!/remake/OptimizationProblem/OptimizationFunction/ReturnCode/MOI  true true
      
      scratch/probe_lost.jl counts 905 names that resolve as OptimizationMOI.<name> on master and not on the PR: 496 from SciMLBase, 152 Symbolics, 141 LinearAlgebra, 59 SymbolicIndexingInterface, 24 SparseArrays, 20 OptimizationBase, 11 SciMLStructures, 2 Reexport.
    • Confirmed by reading downstream source (gh search code + gh api contents). I did not run these packages:
      • JuliaComputing/DyadControlSystems.jl src/MPC/solver_optimization.jl has using OptimizationMOI … sol = OptimizationMOI.solve!(optprob). That is package source with compat OptimizationMOI = "1", so a patch release of this PR (no version bump is proposed) gives an UndefVarError at runtime in MPC solves. DyadLang/DyadControlSystems.jl has the same line.
      • SciML/SciMLBenchmarks.jl benchmarks/OptimizationFrameworks/optimal_powerflow.jmd:804-805 has import OptimizationMOI … OptimizationMOI.solve(prob, Ipopt.Optimizer()). Its Manifest pins OptimizationMOI 1.4.1, which is this package's current version.
    • The PR's own risk section admits that "downstream code spelling them that way would break", but it only scanned lib/, test/ and docs/. CI didn't catch this either. The only downstream job that touches MTK (ModelingToolkit.jl/Optimization, https://github.com/SciML/Optimization.jl/actions/runs/37838170161/job/113520471063) failed in Pkg resolution (Unsatisfiable requirements detected for package TimerOutputs, via Clarabel/ConvexOptimization), so no downstream OptimizationMOI code ran.
    • Required change: add init, solve, solve! to the explicit using SciMLBase: … list next to reinit!, with the same no_stale_explicit_imports ignore entries in test/qa/qa.jl. Also add a Core-test assertion that OptimizationMOI.solve, .solve!, .init and .reinit! are defined, so the next cleanup can't drop them silently. A plain test is needed: the existing OptimizationMOI.reinit! uses sit inside @test_broken begin … end (test/core_tests.jl:251-255, 263-267), which would still record "Broken" if the name were undefined. The alternative is to treat the namespace shrink as breaking (version bump, downstream fixed first). That is Chris's call, not a silent patch.

Non-blocking findings

  1. Runtime eval namespace changed (confirmed by running; no correct result regresses). src/moi.jl:311 does return eval(expr) for numeric-only calls in the HiGHS/MOIOptimizationCache path. That resolves function symbols in the OptimizationMOI namespace at run time, which ExplicitImports can't see. I ran scratch/probe_eval.jl (MTK OptimizationSystem + HiGHS, closed-form minimiser 1/c of c*x^2 - 2x) against both envs:

    • Same result on master and PR: sqrt/exp/abs/log of parameters (correct), scalar norm/dot/tr (correct, they lower to abs/*), max/min/ifelse (MalformedExprException on both).
    • Different: array parameter norm(p) with p = [3, 4]. Master returns u = 0.3333 against the expected 0.2, a silently wrong answer: it evaluates norm(3.0) because array params collapse to their first element (a separate bug that predates this PR). The PR throws UndefVarError: `norm` not defined in `OptimizationMOI` .

    So this turns a silent wrong answer into an error, which is fine. But it changes behaviour, and the PR body doesn't mention it. Any user-reachable call head from LinearAlgebra/SparseArrays/Symbolics exports now errors instead of evaluating.

  2. Reexport is now an unused direct dependency (confirmed by reading). The PR removes using Reexport. On master it was already dead: there was no @reexport. But Project.toml still lists Reexport in [deps] (line 12) and [compat] (line 42). Aqua's stale_deps doesn't flag it because SciMLBase loads Reexport transitively (QA passed 21/21). Drop both entries in this PR, since this PR removes the last reference.

  3. Dead QA config (predates the PR, suspected from reading). The no_stale_explicit_imports ignores :parameters, :toexpr, :unknowns, :varmap_to_vars and all_explicit_imports_are_public = (; ignore = (:varmap_to_vars,)) name things that OptimizationMOI no longer imports at all. The PR adds to this list rather than pruning it. The qa.jl:12 comment ("ignored stale imports are part of the intentionally re-surfaced API") is now only true for reinit!. Not required for this PR.

  4. The orphaned comment at src/OptimizationMOI.jl:3-4 ("Not re-exported: …") is still accurate, since nothing is re-exported. Fine as is.

  5. Consistency with sibling work. The style matches merged OptimizationNLopt: make imports explicit #1398 (OptimizationNLopt) and the open drafts OptimizationBase: make implicit imports explicit, un-break no_implicit_imports QA check #1391, /1392, /1397, /1409, /1411. OptimizationNLopt: make imports explicit #1398 shrank the OptimizationNLopt.* namespace the same way, so whatever policy is chosen for solve/solve!/init here should be applied to those too.

What I ran

All runs used Julia 1.12.7 with TMPDIR set to the review tmp dir.

  • OPTIMIZATION_TEST_GROUP=Core julia --project -e 'using Pkg; Pkg.test()' in lib/OptimizationMOI → Core | 40 2 42 3m00.8s, tests passed. This matches the PR's 40/2/42; the 2 broken are the @test_broken reinit blocks that predate the PR. Log: scratch/core.log.
  • OPTIMIZATION_TEST_GROUP=QA julia --project -e 'using Pkg; Pkg.test()' → Quality Assurance | 21 21 1m22.7s, tests passed. This matches the PR (no_implicit_imports now passes, with no broken entries). Log: scratch/qa.log.
  • julia +1.12 --project=@runic -m Runic --check on both changed files → exit 0. typos on both → exit 0.
  • scratch/probe_globals.jl: walks the lowered code of all 86 methods defined in the PR's OptimizationMOI (found by scanning every loaded module's functions and types) and checks each GlobalRef into the module with isdefined. Result: no undefined globals, so nothing inside the package body lost a binding.
  • scratch/probe_lost.jl: lists the names that master's blanket using exposes and the PR drops (905).
  • scratch/probe_eval.jl against scratch/env_master and scratch/env_pr (both dev the matching OptimizationBase + OptimizationMOI; pathof checked for each) → the results in non-blocking finding 1.
  • isdefined(OptimizationMOI, n) side-by-side for master vs PR → the table in blocking finding 1.
  • gh search code for OptimizationMOI.<name> across GitHub, plus gh api reads of SciMLBenchmarks optimal_powerflow.jmd/Manifest and DyadControlSystems Project.toml/src/MPC/solver_optimization.jl.
  • Read the CI rollup and the logs of both failing jobs. Both are Pkg resolver failures unrelated to this diff: MTK downstream, and downgrade-sublibraries / test (lib/OptimizationReactant).

What I did not verify

  • I didn't run DyadControlSystems or SciMLBenchmarks against the PR. The breakage claim rests on their source text plus the isdefined result above.
  • Julia LTS (1.10) and 1.11. I only ran 1.12.7. CI reports Core passing on lts/1/pre and QA passing on Julia 1.
  • Other downstream packages, which may use dropped names other than solve/solve!/init. The code search found none, but it is not exhaustive and doesn't see private code outside the orgs I can read.
  • The repo has no AGENTS.md or CLAUDE.md at its root, so only the global CLAUDE.md rules applied.
  • Side effect of my verification: Pkg.instantiate created lib/OptimizationMOI/Manifest.toml in the checkout. It is gitignored (git status --ignored shows !!); nothing tracked was modified.

🤖 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