pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Appsi fbbt KeyError when Var bounded by Param

Open emma58 opened this issue 3 years ago • 8 comments

Summary

There's a KeyError when using appsi fbbt on a model where there are Params in the Var bounds

Steps to reproduce the issue

from pyomo.environ import *
from pyomo.contrib import appsi

m = ConcreteModel()
m.p = Param(initialize=4)
m.x = Var(bounds=(0, m.p))
m.c = Constraint(expr=m.x >= 2)

m.pprint()

it = appsi.fbbt.IntervalTightener()
it.perform_fbbt(m)

m.pprint()

Error Message

Traceback (most recent call last):
  File "maybe.py", line 62, in <module>
    it.perform_fbbt(m)
  File "/home/esjohn/src/pyomo/pyomo/contrib/appsi/fbbt.py", line 219, in perform_fbbt
    self.set_instance(model, symbolic_solver_labels=symbolic_solver_labels)
  File "/home/esjohn/src/pyomo/pyomo/contrib/appsi/fbbt.py", line 94, in set_instance
    self.add_block(model)
  File "/home/esjohn/src/pyomo/pyomo/contrib/appsi/base.py", line 914, in add_block
    self.add_variables(list(dict((id(var), var) for var in block.component_data_objects(Var, descend_into=True)).values()))
  File "/home/esjohn/src/pyomo/pyomo/contrib/appsi/base.py", line 787, in add_variables
    self._add_variables(variables)
  File "/home/esjohn/src/pyomo/pyomo/contrib/appsi/fbbt.py", line 107, in _add_variables
    cmodel.process_pyomo_vars(self._pyomo_expr_types, variables, self._var_map, self._param_map,
KeyError: 140146219916720

Note that replacing m.p with value(m.p) works as expected.

Information on your system

Pyomo version: main Python version: 3.8 Operating system: linux How Pyomo was installed (PyPI, conda, source): source Solver (if applicable):

emma58 avatar Oct 24 '22 20:10 emma58

Note: as part of resolving this issue, we should revisit the use of APPSI FBBT in GDPopt (as disabled in #2575)

jsiirola avatar Oct 26 '22 04:10 jsiirola

I just ran into this issue on current main. The same bug affects appsi_cbc and appsi_ipopt.

bknueven avatar Mar 07 '24 04:03 bknueven

And the same thing affects constraint bounds:

from pyomo.environ import *                                                                                

m = ConcreteModel()
m.x = Var()
m.clb = Param(initialize=0)
m.c = Constraint(expr=(m.clb, m.x, None))

SolverFactory("appsi_ipopt").solve(m)

Result:

Traceback (most recent call last):
  File "/Users/bknueven/Software/pyomo_test.py", line 8, in <module>
    SolverFactory("appsi_ipopt").solve(m)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/base.py", line 1568, in solve
    results: Results = super(LegacySolverInterface, self).solve(model)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/solvers/ipopt.py", line 291, in solve
    self._writer.write(model, self._filename + '.nl', timer=timer)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/writers/nl_writer.py", line 240, in write
    self.set_instance(model)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/writers/nl_writer.py", line 76, in set_instance
    self.add_block(model)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/base.py", line 1114, in add_block
    self.add_constraints(
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/base.py", line 1032, in add_constraints
    self._add_constraints(cons)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/appsi/writers/nl_writer.py", line 115, in _add_constraints
    cmodel.process_nl_constraints(
KeyError: 140313948891280

@michaelbynum it seems like the constraint and variable bounds need to go through generate_standard_repn when converted to the cmodel to strip out non-mutable parameters, which don't need to be reflected in the cmodel anyways. I would be happy to implement that fix, but I'm wondering if there's a reason it isn't already done that way.

bknueven avatar Mar 07 '24 15:03 bknueven

@bknueven - request: can you see if the same issue occurs using ipopt_v2? (This pulls the newer interface from pyomo.contrib.solver that we released as a preview)

mrmundt avatar Mar 07 '24 15:03 mrmundt

@bknueven - request: can you see if the same issue occurs using ipopt_v2? (This pulls the newer interface from pyomo.contrib.solver that we released as a preview)

Good question -- ipopt_v2 does not have this bug, though I got an unrelated error when giving it the above model with no objective function.

Traceback (most recent call last):
  File "/Users/bknueven/Software/pyomo_test.py", line 9, in <module>
    SolverFactory("ipopt_v2").solve(m, tee=True)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/solver/base.py", line 518, in solve
    legacy_results, legacy_soln = self._map_results(model, results)
  File "/Users/bknueven/Software/pyomo/pyomo/contrib/solver/base.py", line 422, in _map_results
    if len(list(obj)) > 0:
TypeError: 'NoneType' object is not iterable

obj is None here with no objective.

bknueven avatar Mar 07 '24 15:03 bknueven

The reason this is not accounted for is because I never use non-mutable parameters, so I never think about these edge cases.

I don't think we can run the bounds through generate_standard_repn because that will also remove the mutable parameters (note that mutable parameters in variable bounds work).

This might be fixed just by removing these two if statements: https://github.com/Pyomo/pyomo/blob/3a43b404c86dcd8472c122cab36db8dac7282127/pyomo/contrib/appsi/base.py#L1101 https://github.com/Pyomo/pyomo/blob/3a43b404c86dcd8472c122cab36db8dac7282127/pyomo/contrib/appsi/base.py#L1304

michaelbynum avatar Mar 07 '24 15:03 michaelbynum

Thanks Michael. That does seem to work, unsurprisingly.

What about the following patch? It keeps the non-mutable parameters out of the cmodel, at the cost of checking the mutable attribute.

diff --git a/pyomo/contrib/appsi/cmodel/src/expression.cpp b/pyomo/contrib/appsi/cmodel/src/expression.cpp
index 234ef47e8..8079de42b 100644
--- a/pyomo/contrib/appsi/cmodel/src/expression.cpp
+++ b/pyomo/contrib/appsi/cmodel/src/expression.cpp
@@ -1548,7 +1548,10 @@ appsi_operator_from_pyomo_expr(py::handle expr, py::handle var_map,
     break;
   }
   case param: {
-    res = param_map[expr_types.id(expr)].cast<std::shared_ptr<Node>>();
+    if (expr.attr("parent_component")().attr("mutable").cast<bool>())
+        res = param_map[expr_types.id(expr)].cast<std::shared_ptr<Node>>();
+    else
+        res = std::make_shared<Constant>(expr.attr("value").cast<double>());
     break;
   }
   case product: {

bknueven avatar Mar 07 '24 16:03 bknueven

Looks great to me!

michaelbynum avatar Mar 07 '24 17:03 michaelbynum

FYI, @bknueven , the bug you reported in ipopt_v2 has been corrected and is merged into main.

mrmundt avatar Mar 12 '24 19:03 mrmundt