Skip to content

OptimizationNLPModels: make imports explicit - #1409

Merged
ChrisRackauckas merged 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:qa/nlpmodels-explicit-imports
Oct 10, 2026
Merged

ChrisRackauckas merged 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:qa/nlpmodels-explicit-imports

Conversation

@ChrisRackauckas-Claude

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

Copy link
Copy Markdown
Member

OptimizationNLPModels now imports the NLPModels, ADTypes, OptimizationBase, SciMLBase, and SparseArrays module bindings and the individual names its implementation uses. The QA suite no longer marks no_implicit_imports as broken, and OptimizationNLPModels.OptimizationProblem, NLPModelsAdaptor, build_nlpmodel_meta, and the reexported NLPModels types remain available.

Round 2 (review feedback): dropped unused cons/grad/hess imports from NLPModels (they were only local closures inside OptimizationFunction, not module-level uses) and removed the comments that claimed otherwise / pointed at callers.

Verification

The QA and Core commands below ran from lib/OptimizationNLPModels, with the Julia depot and temporary directory isolated under this job directory. Group env var confirmed in test/runtests.jl as OPTIMIZATION_TEST_GROUP.

  • Baseline QA, before changes:
    export TMPDIR=/home/crackauc/sandbox/goals/qa-hygiene/jobs/nlpm-imports/tmp
    export JULIA_DEPOT_PATH=/home/crackauc/sandbox/goals/qa-hygiene/jobs/nlpm-imports/scratch/julia-depot
    export JULIA_CONDAPKG_BACKEND=Null JULIA_PYTHONCALL_EXE=/usr/bin/python3
    ulimit -n 65536
    OPTIMIZATION_TEST_GROUP=QA julia --project -e 'using Pkg; Pkg.test()'
    Output tail: Quality Assurance | 34 1 35 1m01.1s; Testing OptimizationNLPModels tests passed.
  • QA after change (round 1 and round 2): same commands — round 2 output tail: Quality Assurance | 35 35 1m05.4s; Testing OptimizationNLPModels tests passed.
  • Core (round 2):
    export TMPDIR=/home/crackauc/sandbox/goals/qa-hygiene/jobs/nlpm-imports/tmp
    export JULIA_DEPOT_PATH=/home/crackauc/sandbox/goals/qa-hygiene/jobs/nlpm-imports/scratch/julia-depot
    export JULIA_CONDAPKG_BACKEND=Null JULIA_PYTHONCALL_EXE=/usr/bin/python3
    ulimit -n 65536
    OPTIMIZATION_TEST_GROUP=Core julia --project -e 'using Pkg; Pkg.test()'
    Output tail: Core | 34 34 2m10.1s; Testing OptimizationNLPModels tests passed.
  • From the repository root, julia --project=@runic -e 'using Runic; exit(Runic.main(ARGS))' -- --inplace lib/OptimizationNLPModels/src/OptimizationNLPModels.jl — exit 0 (round 2).
  • From the repository root, typos lib/OptimizationNLPModels/src/OptimizationNLPModels.jl lib/OptimizationNLPModels/test/qa/qa.jl — exit 0.
  • ExplicitImports after dropping cons/grad/hess: check_no_implicit_imports and stale checks return ok.
  • Scanned lib/, test/, and docs/ for qualified OptimizationNLPModels.x references. Code uses OptimizationNLPModels.OptimizationProblem and OptimizationNLPModels.NLPModelsAdaptor in lib/OptimizationNLPModels/test/core_tests.jl; docs mention NLPModelsAdaptor and build_nlpmodel_meta. OptimizationProblem is imported from SciMLBase (owner) and used at the constructor return site so the binding stays live; reexport QA passes.

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

Please ignore this draft until reviewed by @ChrisRackauckas.

Risk assessment

  • Risk: low
  • Blast radius: imports and QA status for OptimizationNLPModels. Exports and docstrings are unchanged. Qualified uses in-repo (OptimizationNLPModels.OptimizationProblem, NLPModelsAdaptor, build_nlpmodel_meta) still resolve. Names that were only incidentally reachable as OptimizationNLPModels.x through the old blanket using no longer are; nothing in lib/, test/ or docs/ uses them, but downstream code spelling them that way would break.
  • Evidence: QA changed from 34 pass / 1 broken to 35 pass (round 2 reconfirm 35/35); Core passes 34/34; Runic and typos exit 0; ExplicitImports reports no implicit imports without the dead cons/grad/hess imports.
  • Independent review: round 1 Claude Opus 5.5 returned CHANGES (dead imports/comment); addressed in round 2. Further review pending.
  • Merge: needs review

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

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

ChrisRackauckas and others added 2 commits October 10, 2026 03:40
Replace blanket `using` with explicit module and name imports so
`no_implicit_imports` can run without an `ei_broken` marker.

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/nlpm-imports/log.txt on amdci2
Those names were only local closures inside OptimizationFunction; ExplicitImports
does not need them. Also trim the misleading import comments.

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/nlpm-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): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

PR: #1409 — "OptimizationNLPModels: make imports explicit" (head 6b01f1e). It changes 2 files, +8/−12. The blanket using NLPModels/ADTypes/OptimizationBase/SparseArrays and import SciMLBase are replaced with explicit imports. The ei_broken = (:no_implicit_imports,) marker and its comment are removed from test/qa/qa.jl. OptimizationBase.OptimizationProblem(...) becomes OptimizationProblem(...), which is imported from SciMLBase. No linked issue, no tracking issue. The repo root has no AGENTS.md or CLAUDE.md.

Blocking findings

None.

The test change is a removal of a broken marker, not a weakening. On the master source, check_no_implicit_imports throws ImplicitImportsException (ADTypes, NLPModels, bound_constrained, cons, grad, has_bounds, …, OptimizationBase, …). On the PR source it returns ok. So the QA check now runs for real and passes; it does not pass trivially (confirmed by running, see below).

Non-blocking findings

  1. The set of names reachable through the module shrinks (confirmed by running). I loaded the master and PR sources side by side (scratch/probe_names.jl) and checked every exported name of NLPModels, ADTypes, OptimizationBase, SciMLBase and SparseArrays:
    • 291 names were reachable as OptimizationNLPModels.x on master and are not on the PR. Examples: solve, obj, grad!, jac_coord!, AutoZygote, MinSense, OptimizationCache, sparse.
    • No names were gained, and the names(OptimizationNLPModels) export list is identical.
    • None of the lost names were exported or documented by this package.
    • In-repo users only touch OptimizationProblem, NLPModelsAdaptor and build_nlpmodel_meta. That covers lib/OptimizationMadNLP (using OptimizationNLPModels: NLPModelsAdaptor, build_nlpmodel_meta), test/core_tests.jl and docs/src/optimization_packages/nlpmodels.md.
    • I don't count this as a SemVer break. The PR body discloses it.
  2. Inconsistent spelling of MaxSense (from reading). MaxSense is imported unqualified only for cache.sense !== MaxSense in build_nlpmodel_meta. The OptimizationProblem constructor (src line 79) still spells it OptimizationBase.MinSense : OptimizationBase.MaxSense. One spelling would be cleaner; this is cosmetic only.
  3. Identity of the swapped bindings (confirmed by running). Both OptimizationBase.OptimizationFunction === SciMLBase.OptimizationFunction and OptimizationBase.OptimizationProblem === SciMLBase.OptimizationProblem are true. So the switch from OptimizationBase.OptimizationProblem(...) to SciMLBase's OptimizationProblem calls the same function. MaxSense is owned by SciMLBase, where it is not public, and is imported from OptimizationBase, where it is public. ExplicitImports via_owners and are_public both return ok for the PR source.
  4. Attribution format falls short of the account rules (process only).
    • The commit trailers say Agent-Model: auto, which is not an exact model ID.
    • The PR body's first line is not the required "UNREVIEWED" banner. It has "Please ignore this draft until reviewed" mid-body and an attribution line at the end instead.
    • The PR is a draft.
  5. The two red CI jobs are unrelated (confirmed from CI logs).
    • ModelingToolkit.jl/Optimization / Downstream Tests fails with a resolver error: "Unsatisfiable requirements detected for package TimerOutputs".
    • downgrade-sublibraries / test (lib/OptimizationReactant) fails with "45 passed, 9 failed, 1 errored". The "Downgrade Sublibraries" workflow is also failing on recent master runs.
    • All OptimizationNLPModels and OptimizationMadNLP jobs are green: Core lts/1/pre, QA, and downgrade.

What I ran

All runs used TMPDIR set to the review tmp directory and the default depot.

  • QA on Julia 1.11 (lib/OptimizationNLPModels, PR head): OPTIMIZATION_TEST_GROUP=QA julia +1.11 --project -e 'using Pkg; Pkg.test()' gave Quality Assurance | 35 35, tests passed. This matches the PR body's 35/35.
  • QA on Julia 1.10.12 (lts), run in a scratch clone at the PR head because the 1.11 manifest blocked lts: Quality Assurance | 33 33, tests passed. The lower count is suspected to be version-gated public-API checks; I did not verify which ones.
  • Core on Julia 1.11: OPTIMIZATION_TEST_GROUP=Core ... gave Core | 34 34, tests passed. This matches the PR body.
  • ExplicitImports, master vs PR (scratch/ei_probe.jl, separate envs that dev each tree's OptimizationNLPModels and OptimizationBase). I ran check_no_implicit_imports, check_no_stale_explicit_imports, check_all_explicit_imports_via_owners and check_all_explicit_imports_are_public:
    • Master: implicit imports throw; the other three are ok.
    • PR: all four are ok.
  • Name-reachability and binding-identity probe: scratch/probe_names.jl. Results are in non-blocking findings 1 and 3.
  • Runic: julia +1.12 --project=@runic -m Runic --check --diff on both changed files exited 0.
  • typos: git diff origin/master...HEAD | typos - exited 0.
  • CI: gh pr view --json statusCheckRollup showed 53 success, 3 skipped and 2 failures; I read the logs of both failures.
  • Cleanup: I removed the untracked lib/OptimizationNLPModels/Manifest.toml that my own test run created in the checkout. No Julia processes from this review are still running.

What I did not verify

  • OptimizationMadNLP tests locally. I relied on CI, where its Core and QA are green.
  • The docs build. No docstrings or docs changed.
  • Downstream packages outside this repo that might spell non-exported names as OptimizationNLPModels.x. I did not search GitHub-wide.
  • QA on Julia pre. Its test_groups.toml lists only lts and 1 for QA.
  • The PR body's "baseline 34 pass / 1 broken" QA number. I confirmed the underlying fact directly with ExplicitImports on master instead of rerunning the master QA suite.

Links


🤖 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)

@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review October 10, 2026 18:13
@ChrisRackauckas
ChrisRackauckas merged commit fe149d1 into SciML:master Oct 10, 2026
56 of 58 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