Closed torressa closed 6 months ago
I don't have much advice here. It was hard to get things working without knowing the internal details. So perhaps I've papered over other mistakes that are now showing themselves.
It's alright Oscar, I only have test_set_basis
left. This a special case as we would need to update before both MOI.get
and MOI.set
. This may lead to some overhead but also to a lot of warning messages. This is the Python API equivalent:
import gurobipy as gp
m = gp.Model()
x = m.addVar()
y = m.addVars(100)
c = m.addConstr(x + y.sum() <= 1)
m.setObjective(-x)
# Update before setting
m.update()
x.VBasis = 0
m.update()
c.CBasis = -1
for i in range(100):
m.update()
y[i].VBasis = -1
# Also update before getting
m.update()
print(x.VBasis)
m.optimize()
This sets the basis correctly but we get a lot (100) of intermediate warnings like:
Warning, invalid warm-start basis discarded
It might just be easier to have the user manually call GRBupdatemodel(model)
in these cases. I will ask Simon.
It might just be easier to have the user manually call GRBupdatemodel(model) in these cases
As a comment, ideally we should not have to do this. The goal of MOI is to abstract across solvers. In the default case, user's won't have access to an object that they can call GRBupdate
on.
Does the Python example throw the same warnings?
When is the warning actually printed?
I think very few people are actually setting the starting basis. But getting is a common operation.
We already update here:
https://github.com/jump-dev/Gurobi.jl/blob/4d204e22cf0f6004e9580583480835c451ebbf97/src/MOI_wrapper/MOI_wrapper.jl#L3691-L3698
So is the only thing missing an update at the start of set
?
https://github.com/jump-dev/Gurobi.jl/blob/4d204e22cf0f6004e9580583480835c451ebbf97/src/MOI_wrapper/MOI_wrapper.jl#L3734-L3752
Does the Python example throw the same warnings?
Yes the warnings are from the Python example, and they will be printed every time we call update without a complete basis so once for every variable except the last one.
I'm okay if we throw the same warnings as Python. But perhaps you could fix this internally to only throw a warning if GRBoptimize
is called without a complete basis?
Attention: Patch coverage is 96.00000%
with 3 lines
in your changes are missing coverage. Please review.
Project coverage is 80.46%. Comparing base (
3a6a83e
) to head (083041e
). Report is 2 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
src/MOI_wrapper/MOI_wrapper.jl | 95.65% | 3 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Simon has a better idea of how to handle this: two kinds of update flags.
attribute_change
in which case we don't have to call GRBupdate
whenever setting further attributes only when GRBget*
as usual;model_change
for any variable, constraint, or objective changes using GRBadd*/GRBdel*
(excluding attribute changes)Let me have another go
@torressa it looks like you have attribute_change=true
for some add_constraint
calls, but attribute_change=false
for others? Which one are you going for?
Just to spell it out to make sure we're on the same page: is the aim here to ensure that an update is done if the user is about to:
?
(Side note: is there really anything to worry about with deletes? Or would JuMP error out earlier than this if the user tries to query an attribute on a deleted variable or constraint?)
@torressa it looks like you have attribute_change=true for some add_constraint calls, but attribute_change=false for others? Which one are you going for?
I did, yes, some add_constraint
functions only change attributes via GRBset*element
, so my thinking was to keep them as attribute type changes (e.g. src/MOI_wrapper/MOI_wrapper.jl#L1897).
Other add_constraint
functions that use GRBadd
or a combination are marked as model changes.
Just to spell it out to make sure we're on the same page: is the aim here to ensure that an update is done if the user is about to:
- get() the value of some attribute for a variable/constraint that was added by the user since the last update call (because get() would otherwise throw an error)
- get() the value of some attribute which was changed by the user since the last update call (because get() would return the old attribute value)
- set() the value of some attribute for a variable/constraint that was added by the user > since the last update call (to handle setting basis values) ?
Exactly, that's the aim.
(Side note: is there really anything to worry about with deletes? Or would JuMP error out earlier than this if the user tries to query an attribute on a deleted variable or constraint?)
I'm not sure what the workflow is in practice, but in the tests, we have things like:
This will fail without updating after deleting
I did, yes, some
add_constraint
functions only change attributes viaGRBset*element
, so my thinking was to keep them as attribute type changes (e.g. src/MOI_wrapper/MOI_wrapper.jl#L1897). Otheradd_constraint
functions that useGRBadd
or a combination are marked as model changes.
Got it, thanks, that makes sense now.
set() the value of some attribute for a variable/constraint that was added by the user > since the last update call (to handle setting basis values)
Great, ok, so _update_if_necessary(model, check_attribute_change = false)
is used only when setting basis statuses. Looks good!
I'm not sure what the workflow is in practice, but in the tests, we have things like:
This will fail without updating after deleting
At least for variables (constraints may need special handling per #516), it seems like a forced update immediately after the delete isn't needed. It should instead need _require_update(model_change=true)
after a delete, so that an update is deferred until after a series of deletes. I'm not really across the details of how MOI handles index tracking on deletes though, so maybe it's better to go with the more paranoid approach for now if this fixes the performance issue you are targeting.
At least for variables (constraints may need special handling per https://github.com/jump-dev/Gurobi.jl/issues/516), it seems like a forced update immediately after the delete isn't needed. It should instead need _require_update(model_change=true) after a delete, so that an update is deferred until after a series of deletes. I'm not really across the details of how MOI handles index tracking on deletes though, so maybe it's better to go with the more paranoid approach for now if this fixes the performance issue you are targeting.
Thanks Simon! You are right, sorry I missed this. One force update remains for deleting a vector of variables (see https://github.com/jump-dev/Gurobi.jl/pull/552#discussion_r1530632683)
@odow if you are also happy with this, I think we can merge it!
Okay. Does this solve #516? Or should we still buffer the deletions?
I think so. I changed some deletions that don't require a forced update (e.g. those that only change an attribute), but the others have to stay as they were to pass the tests (force an update at the end).
Motivated by the large performance difference in Discourse: Problem with JuMP + Gurobi.jl.
The idea is to only call
GRBupdatemodel
beforeGRBget*
only. I've tried to adhere to calling_update_if_required
at the start of the function or at the end. Additionally, adding theforce
argument to this function avoids having to use_require_update
.Some tests are failing still:
test_Buffered_deletion_test
,test_modify_after_delete*
(indexing should also be updated with delete? This is not expected from our other APIs)test_linear_SOS1_integration
test_set_basis
. When setting + getting[V|C]Basis
we need to update beforehand. Doing this on theMOI.set/get
leads to a lot of warning messages and (potentially) performance issues.Work for the update part of https://github.com/jump-dev/Gurobi.jl/issues/516