Conversation
jack-champagne
left a comment
jack-champagne
left a comment
There was a problem hiding this comment.
Looking good so far, reusing IpoptEvaluator for MOI stuff is fine with me! its all MOI.AbstractNLPEvaluator anyways.
A few notes, see my other comments above ofc. Big picture thoughts are that we should move on ripping this out into a package extension for users. A have left some ideas on how to make this easy in my other comments. I have a list of things that we should move around to make this happen:
Move everything MadNLP-specific out of src/solvers/ipopt_solver/:
MadNLPOptionsstruct_solve_madnlp!get_optimizer_and_variables(::DirectTrajOptProblem, ::MadNLPOptions, ...)set_variables!(::MadNLP.Optimizer, ...)update_trajectory!(::DirectTrajOptProblem, ::MadNLP.Optimizer, ...)set_options!(::MadNLP.Optimizer, ::MadNLPOptions)- The
AbstractOptimizerunion type (the Ipopt methods inconstraints.jlcan stay typed toIpopt.Optimizer— the extension will add its own methods or you can widen toMOI.AbstractOptimizer) - The
eval_constraint_jacobian_transpose_productstub - The MadNLP test item(s)
And a list of things that should stay in the ipopt stuff:
IpoptOptions,IpoptEvaluator, all Ipopt solver codeAbstractSolverOptionsinSolversmodule is good- The
solve!method just needs itsoptionstype annotation to remainAbstractSolverOptions— dispatch will route to the right_solve!method based on the concrete type.
src/solvers/ipopt_solver/solver.jl
Outdated
| function _solve!( | ||
| prob::DirectTrajOptProblem, | ||
| options::Any; | ||
| verbose::Bool = true, | ||
| callback = nothing, | ||
| kwargs..., | ||
| ) | ||
| if options isa Solvers.AbstractSolverOptions | ||
| if options isa Solvers.IpoptOptions | ||
| _solve_ipopt!(prob, options; verbose = verbose, callback = callback, kwargs...) | ||
| elseif options isa Solvers.MadNLPOptions | ||
| _solve_madnlp!(prob, options; verbose = verbose, callback = callback, kwargs...) | ||
| else | ||
| @warn "Solver options not recognized" | ||
| end | ||
| else | ||
| @warn "Solver options invalid" | ||
| end | ||
| end |
There was a problem hiding this comment.
The _solve! dispatcher (solver.jl:89-103) checks options isa Solvers.IpoptOptions and options isa Solvers.MadNLPOptions, but both types are defined in IpoptSolverExt, not in the Solvers module (which only exports AbstractSolverOptions and solve!).
The quickest fix is to drop the hand-rolled isa branching entirely and use Julia's dispatch:
_solve!(prob, options::IpoptOptions; kwargs...) = _solve_ipopt!(prob, options; kwargs...)
_solve!(prob, options::MadNLPOptions; kwargs...) = _solve_madnlp!(prob, options; kwargs...)This is more type-d anyways
There was a problem hiding this comment.
This is a huge facepalm moment... I thought I'd already changed Solvers.IpoptOptions and Solvers.MadNLPOptions in the associated struct definitions so they wouldn't have been in IpoptSolverExt at all. Missing that lead to silent-type precompile failures and then the whole messy type branching thing here was an effort to work around that... Claude was also quite confused by that... Anyhow this should be gtg shortly then as long as CI tests are passing more or less like local tests. Atm I think there may still be a linear constraints issue/failed test involving test_constraint, (or possibly just flaky test) but other than that, all good. Will ping when done throwing the MadNLP stuff in its own extension submodule
src/solvers/ipopt_solver/solver.jl
Outdated
| solve!(prob; max_iter = 100) | ||
| end | ||
|
|
||
| @testitem "testing MadNLP.jl solver" begin |
There was a problem hiding this comment.
This test calls _solve_madnlp! directly instead of going through solve!(prob; options=MadNLPOptions(), ...). We should test the public API or make another test with that exact goal.
There was a problem hiding this comment.
Yes, will be fixed in the final
There was a problem hiding this comment.
We should split off the madnlp stuff from the ipopt_solver files and also figure out how we can take the line https://github.com/harmoniqs/DirectTrajOpt.jl/pull/63/changes#diff-f19c825aceabbf5309ddaed77474ba8474413cfc2b7b3d0d950f658d7fda7dafR20 and put it in a different part of the codebase, perhaps the right stuff ends up into a dedicated package extension for MadNLP.
There was a problem hiding this comment.
Ykno we may as well just use MOI.AbstractOptimizer for this.
There was a problem hiding this comment.
if we want to make this a package extension, we can start here and do something like:
[deps]
# remove MadNLP from here
[weakdeps]
MadNLP = "..."
[extensions]
MadNLPExt = "MadNLP"and in file structure:
ext/
MadNLPExt.jl # or ext/MadNLPExt/MadNLPExt.jl if multiple files
the extension module file itself will have to be something like:
module MadNLPExt
using DirectTrajOpt
import DirectTrajOpt: DirectTrajOptProblem, AbstractSolverOptions, ...
import MadNLP
using MathOptInterface
import MathOptInterface as MOI
# MadNLPOptions, _solve_madnlp!, get_optimizer_and_variables, etc.
end
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… Piccolo pre-finalizing the PR
…ng any solver-specific extension code
…oice of default solver at compile time)
…terface.Optimizer
2 participants
Initial setup for MadNLP integration
TODOs
eval_constraint_jacobian_transpose_productis called by MadNLPSolver after optimization has concluded. This seems to pertain to how the NLP is constructed, as a "direct" method of specifying the NLP avoids this issue, even on larger problems. To reviewers and/or Claude: consider instantiating the optimizer inget_optimizer_and_variablesonly after all NLP constraints have been added to the underlying NLPBlock.DirectTrajOpt.Solvers.MadNLPOptions/DirectTrajOpt.IpoptSolverExt.MadNLPOptionsand development of associated notions of anAbstractOptimizertype associated to anAbstractSolverOptionstype. This would be recommended if one wishes to support passing callbacks, options, etc. through a similar API to both solvers.Further goals