Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Mar 17, 2022

Closes jump-dev/JuMP.jl#2931

This was actually slightly more complicated that expected (there's a long comment in the code explaining why this is necessary). But essentially we were missing a test for the case where we tried to copy to a Bridges.full_bridge_optimizer(NL.Model(), Float64). The other file formats work fine because they use @model, but NL is special because I cut out an extra cache and only implemented copy_to. (I can't have tried it via JuMP since I removed the cache.)

Other fixes, like JuMP using a CachingOptimizer, won't work because NL.Model is not an AbstractOptimizer. This is definitely a hole in the API, but since this is a one-off case we can keep it like this for now. If we encounter other places where this is useful, we could consider adding a CachingModel to MOI.Utilities.

julia> using JuMP

julia> model = Model()
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached.

julia> @variable(model, x >= 0)
x

julia> @objective(model, Min, 2x + 1)
2 x + 1

julia> write_to_file(model, "test.nl")

shell> cat test.nl
g3 1 1 0
 1 0 1 0 0 0
 0 1
 0 0
 0 0 0
 0 0 0 1
 0 0 0 0 0
 0 1
 0 0
 0 0 0 0 0
O0 0
n1
x1
0 0
b
2 0
G0 1
0 2

(copt) pkg> st
      Status `/private/tmp/copt/Project.toml`
  [4076af6c] JuMP v0.23.2
  [b8f27783] MathOptInterface v1.1.0 `~/.julia/dev/MathOptInterface`

@blegat
Copy link
Member

blegat commented Mar 20, 2022

IMO, the restriction for the optimizer of a CachingOptimizer to be a subtype of AbstractOptimizer should be removed.
It does not serve any purpose. I guess we were waiting for a use case like this one to remove it.

@odow
Copy link
Member Author

odow commented Mar 20, 2022

Yeah that could be a different pull request. It might lead to some weird errors, but should be okay for most things.

However, can we merge this first? That lets JuMP 1.0 depend on MOI 1.0. We can drop the restriction in a future version of MOI, and then change JuMP to use that if/when we want to bump the minimum supported version of MOI.

@blegat
Copy link
Member

blegat commented Mar 21, 2022

That lets JuMP 1.0 depend on MOI 1.0. We can drop the restriction in a future version of MOI, and then change JuMP to use that if/when we want to bump the minimum supported version of MOI.

Why would it ? If JuMP depends on this PR, it also means that it will depend on MOI v1.1 no ?

@odow
Copy link
Member Author

odow commented Mar 21, 2022

Its a bug-fix. JuMP's code doesn't need to change.

@blegat
Copy link
Member

blegat commented Mar 21, 2022

JuMP's code doesn't need to change if we allow MOI.ModelLike as optimizer field. It's just more permissive, any previous code will still work, wouldn't it ?

@odow
Copy link
Member Author

odow commented Mar 22, 2022

Because JuMP either needs to explicitly construct a caching optimizer, or call MOI.instantiate, and it can't do either of those with MOI 1.0:

julia> MOI.instantiate(MOI.FileFormats.NL.Model)
ERROR: The provided `optimizer_constructor` returned an object of type MathOptInterface.FileFormats.NL.Model. Expected a MathOptInterface.AbstractOptimizer.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] _instantiate_and_check(optimizer_constructor::Type{MathOptInterface.FileFormats.NL.Model})
   @ MathOptInterface ~/.julia/packages/MathOptInterface/IQWV4/src/instantiate.jl:66
 [3] instantiate(optimizer_constructor::Type; with_bridge_type::Nothing)
   @ MathOptInterface ~/.julia/packages/MathOptInterface/IQWV4/src/instantiate.jl:119
 [4] instantiate(optimizer_constructor::Type)
   @ MathOptInterface ~/.julia/packages/MathOptInterface/IQWV4/src/instantiate.jl:119
 [5] top-level scope
   @ REPL[2]:1

@blegat
Copy link
Member

blegat commented Mar 22, 2022

If we additionally fix instantiate to check for MOI.ModelLike and not for MOI.AbstractOptimizer then we don't need any change to JuMP, right ?
I feel there is nothing wrong with what NL.Model is doing and we might encounter other similar issues if we don't fix the root cause.

@odow
Copy link
Member Author

odow commented Mar 22, 2022

Okay. Let me put together a PR with the full fix. Maybe we do just lower-bound JuMP to MOI 1.1.2

@odow
Copy link
Member Author

odow commented Mar 23, 2022

Closing in favor of #1781

@odow odow closed this Mar 23, 2022
@odow odow deleted the od/fix-nl branch March 23, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Writing to NL file fails

2 participants