Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Jun 23, 2019

Some operations do not fall into the category of set such as:

So when a solver want to expose a solver specific feature like this, we need to create a new add_... function in MOI and implement it in every layer + adding a function to JuMP, ...
This PR adds a MOI.add function which is similar to MOI.set except it returns an AttributeIndex which is consistent with add_variable(s) and add_constraint(s).
With this PR, if a solver want to expose, say lazy constraints, it just have to define LazyConstraint <: MOI.AbstractOptimizerAttribute, implement MOI.add(::Optimizer, ::LazyConstraint, ...) and no change is needed either from JuMP or MOI.

This might be the only breaking change needed to add support for callbacks so it might be worth it to include it into v0.9

@blegat blegat added this to the v0.9 milestone Jun 23, 2019
@blegat blegat requested review from mlubin and odow June 23, 2019 14:47
Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea. A few comments:

I think overloading the existing Abstract*Attribute for this is confusing. For example, LazyConstraint could be both a constraint flag (http://www.gurobi.com/documentation/8.1/refman/lazy.html) describing how a regular constraint should be handled by a solver or a constraint added in a callback. I'd rather have an entirely separate abstraction for submitting "things" to the solver.

Lazy constraints never receive indices that you can later refer to them with (AFAIK), so we could drop AttributeIndex.

The assumption that you only need to set variable-wise values is a bit off. What if you want to add a second-order cone constraint as a lazy constraint?

I'd propose something like:

submit(model::ModelLike, ::AbstractSubmittableThing, ::Any)

Each AbstractSubmittableThing decides what the SubmittablePayload should be. For LazyConstraint it could be function-set tuple. For heuristic solutions it could be a dictionary from VariableIndex to value. The naming needs some work.

I can only imagine this working in direct mode, because it would be hard to translate indices between the cache and the underlying solver given arbitrary payloads.

Also it wasn't clear to me if LazyConstraint in the proposal means a callback object or the actual constraint that would be added inside a callback.

@codecov-io
Copy link

codecov-io commented Jun 23, 2019

Codecov Report

Merging #775 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   94.26%   94.14%   -0.12%     
==========================================
  Files          59       59              
  Lines        6481     6489       +8     
==========================================
  Hits         6109     6109              
- Misses        372      380       +8
Impacted Files Coverage Δ
src/attributes.jl 85.54% <0%> (-9.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd88e5d...ad650f9. Read the comment docs.

@blegat
Copy link
Member Author

blegat commented Jun 23, 2019

The assumption that you only need to set variable-wise values is a bit off. What if you want to add a second-order cone constraint as a lazy constraint?

You pass it as a tuple (func, set) in the value argument.

I can only imagine this working in direct mode, because it would be hard to translate indices between the cache and the underlying solver given arbitrary payloads.

We could have a map_indices that should be implemented for payload containing indices but that can be left as future work. Having this work in direct mode is a good start.

Also it wasn't clear to me if LazyConstraint in the proposal means a callback object or the actual constraint that would be added inside a callback.

The constraint object as (func, set) tuple.

I'd propose something like: submit...

IMO MOI.add should return an index for consistency with MOI.add_variable, ... If we want something without indices, MOI.submit might indeed be a better name. The targeted application is callbacks so if they usually don't return indices, this PR should add MOI.submit and we'll see if we need to define a MOI.add in the future.

@mlubin
Copy link
Member

mlubin commented Jun 23, 2019

The targeted application is callbacks so if they usually don't return indices

They don't usually return indices.

The constraint object as (func, set) tuple.

Then what is the use case for the variable-wise and constraint-wise versions? Submitting a feasible solution?

  add(model::ModelLike, attr::AbstractVariableAttribute, v::VariableIndex,
         value)::AttributeIndex{typeof(attr)}
  add(model::ModelLike, attr::AbstractConstraintAttribute, c::ConstraintIndex,
          value)

@blegat
Copy link
Member Author

blegat commented Jun 23, 2019

Then what is the use case for the variable-wise and constraint-wise versions? Submitting a feasible solution?

Yes, submitting a heuristic solution.

@mtanneau
Copy link
Contributor

I'm only familiar with MILP callbacks so what follows is biased towards this use case; I have no experience (nor knowledge thereof) of callbacks for non-linear optimization solvers.

Some general background. MILP solvers allow you to do two types of things during with a callback:

  • Query some information from the solver, e.g. number of solutions, node statistics, time, etc... No information is pushed to the solver.
    In terms of implementation, all this should be doable via MOI's get + the right attributes. I strongly suggest that any attribute being called within a callback gets the prefix Cb, e.g. CbNodeIndex. That would avoid some confusion.
  • Submit/push/post/provide (whatever word sounds best) information to the solver, e.g.: heuristic solution, lazy constraint, user cut, branching decision, etc... This does affect the solver's behaviour.
    Which information can be passed, is which form, and from where in the resolution process, is solver-specific (although MILP solvers share some common ground). For instance, you typically can't query dual variables if your callback is called after a heuristic found an integer solution.

@mtanneau
Copy link
Contributor

mtanneau commented Jun 25, 2019

Now the specifics:

Heuristic solution

This allows you to suggest a heuristic solution to the solver.
Some will require you to provide a full solution, some allow to provide partial solutions (e.g. integer assignment on a subset of the variables).

This does not modify the model. It does not create a new variable. Your heuristic solution may not even be considered by the solver. Therefore, the submit proposal of Miles seems more appropriate than tweaking the add function.

IMO, an intuitive syntax could look like

MOI.submit(model, ::MOI.CbHeuristicSolution, ::Any)

Lazy constraint and user cut

Provide an additional constraint to the solver, which were not part of the original formulation.
The semantic difference between user cuts and lazy constraints is the following:

  • A lazy constraint may cut an integer solution that was feasible w.r.t. the original formulation.
  • A user cut may not cut any integer feasible solution. If it does, things may break.

Similar to heuristic solution, user cuts and lazy constraints may never enter the formulation (if the solver decides so).

  • Lazy constraints / user cuts are not indexed
  • They may even not be enforced: Gurobi's docs states that

Node solutions will usually respect previously added lazy constraints, but not always.

  • You cannot query the associated dual variables (because they may not even exist)
  • I do not know whether lazy constraints / user cuts are kept when copying a model

The syntax proposed by Miles also fits this nicely, e.g.:

MOI.submit(model, ::MOI.CbUserCut, ::Any)
MOI.submit(model, ::MOI.CbLazyConstraint, ::Any)

the third argument will most likely encapsulate the constraint itself (func, set), and some callback-related data.

Note: as mentioned by Miles, some of the initial constraints may be marked as lazy: http://www.gurobi.com/documentation/8.1/refman/lazy.html. This has nothing to do with callbacks.

Adding a new variable (from a callback)

AFAIK, no MILP solver supports it. The only exception may be SCIP, but I'm not familiar enough.

@mtanneau
Copy link
Contributor

mtanneau commented Jun 25, 2019

Now that I think about it, maybe we should be returning an index when adding constraints (and possibly variables) through a callback. Just because existing MILP solvers don't do it doesn't mean MOI should not, and it would restrict the future capabilities, if someone wants to implement lazy constraints a bit differently.

My idea would look like the following.
There could be an InvalidConstraintIndex, which -as its name suggests- is always invalid. By default, this is the index that would be returned when adding a lazy constraint / user cut via a callback. If a solver wishes to return a valid index because they support it, they can.

The same thing could be done with variables. It would actually be quite useful in a branch-and-price context: new variables are added at each node, but one may want to remove some when taking certain branching decisions.

@mlubin
Copy link
Member

mlubin commented Jun 29, 2019

@odow thoughts?

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like submit. I also support returning indices from MOI.submit. It demonstrates that things are different from the standard get/set, and as @mtanneau says, just because current MILP solvers don't support callbacks, doesn't mean that new solvers won't (e.g., Tulip.jl?).

One question is whether to return an InvalidConstraintIndex, or to return nothing if the solver doesn't support indexing submitted attributes.

attr::AttrType
message::String # Human-friendly explanation why the attribute cannot be set
end
AddAttributeNotAllowed(attr::AnyAttribute) = AddAttributeNotAllowed(attr, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a more descriptive default message?

Copy link
Member Author

@blegat blegat Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a more descriptive one printed by NotAllowedError, the message field is for custom messages that is added at the end of it.

@blegat
Copy link
Member Author

blegat commented Jul 3, 2019

@mlubin @mtanneau @odow Just iterated based on your comments. Let me know what you think of the new design.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like submit. It would be nice to see some examples.

@mlubin mlubin changed the title Add MOI.add to add attribute Add MOI.submit to add attribute Jul 3, 2019
@mlubin
Copy link
Member

mlubin commented Jul 3, 2019

I'm skeptical of the need for SubmittedIndex given that the use cases for it are only hypothetical and could possibly never materialize. It's clunky to carry around pieces of code that won't be used. C.f. https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it.

It would be a different story if we had a solver developer who said, "I want to use this for X."

@blegat
Copy link
Member Author

blegat commented Jul 3, 2019

It might also not be too hard to add later when it's needed so I am ok to not have it for now.

@mtanneau
Copy link
Contributor

mtanneau commented Jul 8, 2019

I'm skeptical of the need for SubmittedIndex given that the use cases for it are only hypothetical and could possibly never materialize.

Miles has a strong point.
In this direction, I can think of only two hypothetical cases/solvers where one could want to support indices for submitted stuff: Pajarito and Coluna. And that's assuming that add_constraint and add_variable were not satisfactory.

In the mean time, it indeed seems preferable not to carry SubmittedIndex around, but add it later if needed.

@blegat blegat force-pushed the bl/add_attribute branch from 97e6008 to 92ca5cc Compare July 9, 2019 09:04
@blegat
Copy link
Member Author

blegat commented Jul 9, 2019

@mlubin @mtanneau @odow Just removed the SubmittedIndex. Any more comments ?

@mtanneau
Copy link
Contributor

mtanneau commented Jul 9, 2019

@mlubin @mtanneau @odow Just removed the SubmittedIndex. Any more comments ?

Looks good. I think what we need now are a couple of examples to check whether something's missing/odd.

I'd be happy to port the examples that were in JuMP v18 (heuristic, lazy & user) to MOI, but I don't have time to commit to this until the end of the month.

@mlubin
Copy link
Member

mlubin commented Jul 9, 2019

I think what we need now are a couple of examples to check whether something's missing/odd.

This PR doesn't have anything specific for callbacks, so I don't think it needs to be blocked until we have a full callback example.

@blegat blegat merged commit ee75849 into master Jul 10, 2019
@blegat blegat deleted the bl/add_attribute branch July 14, 2019 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants