Observer gurobi refactor

[michaelbynum]

Summary/Motivation:

This PR refactors the gurobi interfaces to achieve two goals:

• reduce duplicate code
• use the new contrib.observer package

Changes proposed in this PR:

• create a directory for gurobi now that there are 4 files
• create a base class for all of the interfaces that use gurobipy, making the direct and persistent interfaces more consistent (e.g., tc_map, postsolve, solve, mipstart)
• modify the persistent interface to use the contrib.observer package
• add mipstart functionality to gurobi direct and gurobi minlp
• add a minimum gurobipy version to the direct interface (addMConstr was not added until version 9)
• simplified some of the persistent interface

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[codecov]

Codecov Report

❌ Patch coverage is 91.06977% with 96 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.47%. Comparing base (f2d23a5) to head (7c7be8d).
⚠️ Report is 153 commits behind head on main.

Files with missing lines	Patch %	Lines
...contrib/solver/solvers/gurobi/gurobi_persistent.py	89.53%	83 Missing ⚠️
...ontrib/solver/solvers/gurobi/gurobi_direct_base.py	93.19%	10 Missing ⚠️
...omo/contrib/solver/solvers/gurobi/gurobi_direct.py	97.61%	2 Missing ⚠️
...ntrib/solver/solvers/gurobi/gurobi_direct_minlp.py	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3698      +/-   ##
==========================================
- Coverage   89.48%   89.47%   -0.01%     
==========================================
  Files         904      906       +2     
  Lines      105385   105473      +88     
==========================================
+ Hits        94299    94368      +69     
- Misses      11086    11105      +19     

Flag	Coverage Δ
builders	29.08% <23.06%> (+0.04%)	⬆️
default	83.56% <23.34%> (?)
expensive	35.56% <23.06%> (?)
linux	86.77% <91.06%> (-2.44%)	⬇️
linux_other	86.77% <91.06%> (+0.01%)	⬆️
oldsolvers	29.73% <23.06%> (+0.04%)	⬆️
osx	82.93% <91.06%> (+0.01%)	⬆️
win	85.00% <91.06%> (+<0.01%)	⬆️
win_other	85.00% <91.06%> (+<0.01%)	⬆️
xpress95	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jsiirola]

Finally getting back to this. I think there are still a number of open questions from previous reviews, plus a new one:

[jsiirola]

I am struggling with the design here. I think that the SolutionLoader is filling multiple roles:

• it is the definitive "mapping object" between the Pyomo and Gurobi models
• it knows something about it's own validity (for persistent)
• it has the logic for translating the values back and forth
• it knows when to delete the Gurobi model

I feel like the the persistent interface should just hold on to the SolutionLoader as the definitive "mapping object" between the Pyomo and Gurobi models? That would resolve the issue around premature deletion of the Gurobi model. But it would make invalidation more complicated (because each solution would have the same loader?).

I wonder: would a better design be for a mapping object that currently does 99% of the current SolutionLoader? It would be the "owner" of the Gurobi model reference, know the translation keys, and know an "id" for the current solution. The SolutionLoader would then be a thin wrapper that stored the (mapper, solution_id). Invalidating a SolutionLoader would only require incrementing the solution id in the mapper object (much in the same way that we handle stale for variables). The mapping object could also be used for things like callbacks, too (see @emma58's comment below).

[michaelbynum]

I created issues for the unresolved comments. One is minor (handling Env), but will require writing some new tests. The other has to do with the mapper objects. That will require some major changes that I think we should do as a separate PR.

[jsiirola]

I was going to approve and leave all these comments to a future PR, but I think there is a significant bug that should be resolved before merging: there is inconsistent use var vs id(var) in gurobi_persistent: it looks liek the _pyomo_var_to_solver_var_map has moved to a ComponentMap, but there are still numerous cases where we are storing / looking things up by id().

[jsiirola]

Does it make sense to merge these into get_reduced_costs similar to what was done with get_primals?

[michaelbynum]

I don't think so. There is not really any duplicate code for the reduced costs.

[michaelbynum]

I was going to approve and leave all these comments to a future PR, but I think there is a significant bug that should be resolved before merging: there is inconsistent use var vs id(var) in gurobi_persistent: it looks liek the _pyomo_var_to_solver_var_map has moved to a ComponentMap, but there are still numerous cases where we are storing / looking things up by id().

It's not actually a bug. With ComponentMap, you can key by variable or by id of the variable, and you get the same thing:

In [1]: import pyomo.environ as pe

In [2]: from pyomo.common.collections import ComponentMap

In [3]: m = pe.ConcreteModel()

In [4]: m.x = pe.Var()

In [5]: d = ComponentMap()

In [6]: d[m.x] = 'foo'

In [7]: d[id(m.x)]
Out[7]: 'foo'

That being said, it does need fixed, so I'll fix it now. Tests would have caught most of these if it had been a real bug.

[jsiirola]

It's not actually a bug. With ComponentMap, you can key by variable or by id of the variable, and you get the same thing:

Actually, it is a bug - with ComponentMap. That is actually a hash collision between unhashable types (like Var) and ints. Note also that it only happens for unhashable types. Hashable types (like Constraint / Model) stored as ComponentMap keys cannot be looked up by id().

I had found/fixed it back when we were working on performance in ComponentMap / ComponentSet, but apparently that is still living in a stash somewhere. I will try and dig it up and put up a PR.

[michaelbynum]

Okay, this should be good to go, assuming tests pass.