Refactor GRBupdatemodel calls

[torressa]

Motivated by the large performance difference in Discourse: Problem with JuMP + Gurobi.jl.

The idea is to only call GRBupdatemodel before GRBget* only. I've tried to adhere to calling _update_if_required at the start of the function or at the end. Additionally, adding the force 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 the MOI.set/get leads to a lot of warning messages and (potentially) performance issues.

Work for the update part of #516

[odow]

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.

[torressa]

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.

[odow]

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.

[odow]

We already update here:

Gurobi.jl/src/MOI_wrapper/MOI_wrapper.jl

Lines 3691 to 3698 in 4d204e2

	function MOI.get(
	model::Optimizer,
	::MOI.VariableBasisStatus,
	x::MOI.VariableIndex,
	)
	_update_if_necessary(model)
	valueP = Ref{Cint}()
	ret = GRBgetintattrelement(model, "VBasis", c_column(model, x), valueP)

So is the only thing missing an update at the start of set?

Gurobi.jl/src/MOI_wrapper/MOI_wrapper.jl

Lines 3734 to 3752 in 4d204e2

	function MOI.set(
	model::Optimizer,
	::MOI.VariableBasisStatus,
	x::MOI.VariableIndex,
	bs::MOI.BasisStatusCode,
	)
	valueP = if bs == MOI.BASIC
	Cint(0)
	elseif bs == MOI.NONBASIC_AT_LOWER
	Cint(-1)
	elseif bs == MOI.NONBASIC_AT_UPPER
	Cint(-2)
	else
	@assert bs == MOI.SUPER_BASIC
	Cint(-3)
	end
	ret = GRBsetintattrelement(model, "VBasis", c_column(model, x), valueP)
	_check_ret(model, ret)
	_require_update(model)

[torressa]

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.

[odow]

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?

[codecov]

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   80.36%   80.46%   +0.09%     
==========================================
  Files           6        6              
  Lines        2908     2928      +20     
==========================================
+ Hits         2337     2356      +19     
- Misses        571      572       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[odow]

This doesn't change any tests and everything is still passing so LGTM.

[torressa]

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

[simonbowly]

@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:

1. 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)
2. 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)
3. 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)

?

(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]

@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:

Gurobi.jl/test/MOI/MOI_wrapper.jl

Lines 285 to 303 in 69d2d6e

	model = Gurobi.Optimizer(GRB_ENV)
	MOI.set(model, MOI.Silent(), true)
	vars = MOI.add_variables(model, 3)
	vars_obj = [7.0, 42.0, -0.5]
	obj_attr = Gurobi.VariableAttribute("Obj")
	MOI.set.(model, obj_attr, vars, vars_obj)
	@test all(MOI.is_valid.(model, vars))
	@test MOI.get.(model, obj_attr, vars) == vars_obj
	MOI.delete(model, vars[[1, 3]])
	@test !MOI.is_valid(model, vars[1])
	@test !MOI.is_valid(model, vars[3])
	fourth_var = MOI.add_variable(model)
	fourth_var_obj = -77.0
	MOI.set(model, obj_attr, fourth_var, fourth_var_obj)
	# Check before updating the model and after the delete+insert.
	@test !MOI.is_valid(model, vars[1])
	@test !MOI.is_valid(model, vars[3])
	@test MOI.is_valid(model, vars[2])
	@test vars_obj[2] == MOI.get(model, obj_attr, vars[2])

This will fail without updating after deleting

[simonbowly]

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.

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:

Gurobi.jl/test/MOI/MOI_wrapper.jl

Lines 285 to 303 in 69d2d6e

	model = Gurobi.Optimizer(GRB_ENV)
	MOI.set(model, MOI.Silent(), true)
	vars = MOI.add_variables(model, 3)
	vars_obj = [7.0, 42.0, -0.5]
	obj_attr = Gurobi.VariableAttribute("Obj")
	MOI.set.(model, obj_attr, vars, vars_obj)
	@test all(MOI.is_valid.(model, vars))
	@test MOI.get.(model, obj_attr, vars) == vars_obj
	MOI.delete(model, vars[[1, 3]])
	@test !MOI.is_valid(model, vars[1])
	@test !MOI.is_valid(model, vars[3])
	fourth_var = MOI.add_variable(model)
	fourth_var_obj = -77.0
	MOI.set(model, obj_attr, fourth_var, fourth_var_obj)
	# Check before updating the model and after the delete+insert.
	@test !MOI.is_valid(model, vars[1])
	@test !MOI.is_valid(model, vars[3])
	@test MOI.is_valid(model, vars[2])
	@test vars_obj[2] == MOI.get(model, obj_attr, vars[2])

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.

[torressa]

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.

Thanks Simon! You are right, sorry I missed this.
One force update remains for deleting a vector of variables (see #552 (comment))

[torressa]

This is the only update remaining for variable deletion as is needed for the test_Buffered_deletion_test case (linked earlier)

[torressa]

@odow if you are also happy with this, I think we can merge it!

[odow]

Okay. Does this solve #516? Or should we still buffer the deletions?

[torressa]

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