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.
| 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
| 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.
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.
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.
end2 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