Add branching priorities to CPLEXSHELL #1300

ruaridhw · 2020-02-19T18:45:50Z

Fixes #1299

Summary/Motivation:

To be able to use the branching priority capability of commercial solvers when solving MIP problems

Changes proposed in this PR:

Add branching priority / direction data to variables
Add branching priority capability to the CPLEXSHELL interface

Legal Acknowledgement

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

I agree my contributions are submitted under the BSD license.
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.

- Include `branch_priority` and `branch_direction` attributes of variable data - Read off these attributes as part of the CPLEXSHELL solve and write them to a CPLEX .ord file - Update the CPLEX OPL script to read these .ord file should it exist

- End-to-end tests of `opt.solve()` using branching priorities with CPLEXSHELL

- Declare `priority` and `direction` suffixes on the model instead of attributes on the variable data in order to represent branching decisions

codecov · 2020-02-21T08:51:54Z

Codecov Report

Merging #1300 into master will increase coverage by 2.92%.
The diff coverage is 96.38%.

@@            Coverage Diff            @@
##           master   #1300      +/-   ##
=========================================
+ Coverage   68.57%   71.5%   +2.92%     
=========================================
  Files         534     540       +6     
  Lines       81026   87389    +6363     
=========================================
+ Hits        55567   62486    +6919     
+ Misses      25459   24903     -556

Impacted Files	Coverage Δ
pyomo/opt/base/problem.py	`94.11% <100%> (+2.45%)`	⬆️
pyomo/util/components.py	`100% <100%> (ø)`	⬆️
pyomo/solvers/plugins/solvers/CPLEX.py	`88.7% <95.52%> (+3.82%)`	⬆️
pyomo/solvers/plugins/solvers/gurobi_direct.py	`60.24% <0%> (-10.76%)`	⬇️
examples/gdp/jobshop.py	`91.3% <0%> (-8.7%)`	⬇️
pyomo/pysp/plugins/csvsolutionwriter.py	`95.34% <0%> (-1.63%)`	⬇️
pyomo/contrib/pynumero/sparse/intrinsic.py	`86.66% <0%> (-1.54%)`	⬇️
pyomo/pysp/phinit.py	`54.38% <0%> (-1.18%)`	⬇️
pyomo/contrib/gdpopt/iterate.py	`77.41% <0%> (-0.77%)`	⬇️
pyomo/opt/testing/pyunit.py	`28.94% <0%> (-0.29%)`	⬇️
... and 74 more

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 bf7daa3...456cb86. Read the comment docs.

- Looks like there's some circular imports failing specifically for Py2.7 - Follow the same (hacky) local import design chosen in 0935e75

- Update tests to avoid usage of "magic numbers" and reference `BranchDirection` where possible

ruaridhw · 2020-02-24T08:35:20Z

This should be ready for review as all the checks are passing now. GitHub is giving me a few suggestions (@ghackebeil, @jsiirola, @whart222) though I'm not sure who would be best placed so whenever one of you is free!

michaelbynum

The implementation looks good to me. My only real concern is whether or not we want to support this type of functionality through a file-based interface. Cplex has many, many options and configurations, and if we were to add support for all of them, the interface would be terribly complex. On the other hand, we could focus our effort on improving the direct/persistent interface, where it would be very easy to support almost any Cplex option/configuration. Thoughts @jsiirola @carldlaird @jwatsonnm @ghackebeil @DLWoodruff ?

jsiirola · 2020-03-04T17:36:20Z

pyomo/solvers/tests/checks/test_cplex.py

+        from pyomo.solvers.plugins.converter.model import PyomoMIPConverter  # register the `ProblemConverterFactory`
+        from pyomo.repn.plugins.cpxlp import ProblemWriter_cpxlp  # register the `WriterFactory`


Instead if explicitly importing the plugins here, I think it would be preferable to just

import pyomo.environ

at the top of the module (which will ensure that standard plugins are registered.

jsiirola · 2020-03-04T17:39:08Z

pyomo/solvers/tests/checks/test_cplex.py

+            "* ENCODING=ISO-8859-1\nNAME             Priority Order\n %s x1 %d\nENDATA\n"
+            % (ORDFileSchema._direction_to_str(direction_val), priority_val),


I think I would prefer this string to be hard-coded (not generated with %s placeholders). Doing so will ensure that this also tests that the ORDFileSchema._direction_to_str is correct.

Sure. I've updated this and the below

Below got updated, but I was thinking something closer to this here:

self.assertEqual( priorities_file, "* ENCODING=ISO-8859-1\n" "NAME Priority Order\n" " DN x1 10\n" "ENDATA\n")

jsiirola · 2020-03-04T17:40:34Z

pyomo/solvers/tests/checks/test_cplex.py

+            "* ENCODING=ISO-8859-1\nNAME             Priority Order\n  x1 %d\nENDATA\n"
+            % (priority_val,),


Same as above: I would recommend hard-coding the reference string.

jsiirola · 2020-03-04T17:43:39Z

pyomo/opt/base/problem.py

@@ -44,3 +50,12 @@ def __enter__(self):
    def __exit__(self, t, v, traceback):
        pass

+
+class BranchDirection(object):


Open question: is this the best place for this enum-like object?

I wondered the same thing, but I'm not sure of a better place for it. Gurobi has the same options (up, down, and default).

Yeah, this was the only place general enough that I could think of as there's nothing else as suitable. We could always introduce a new module for this though?

ruaridhw · 2020-03-04T17:49:19Z

Thanks @michaelbynum. I am certainly up for improving the direct/persistent interface as well and would be happy to raise a separate PR to implement this functionality for that.

As it happens, we were previously using CPLEXPersistent for this advanced functionality and found it to be a little unreliable (see known issues such as #1244, #1169) hence the (hopefully temporary) move back to CPLEXSHELL and this PR.

With regards to supporting advanced functionality in CPLEXSHELL, from what I can see, there are only a handful of file-based interfaces that pyomo doesn't already support. Personally, the only other input format that would be much use would be "ANN" for model annotations. I understand that all other settings and parameters are already exposed by pyomo via solver.options.

michaelbynum · 2020-03-07T18:48:47Z

@ruaridhw Good points. I am working on improving the direct/persistent solvers separately. This is a good solution for now. If you can address @jsiirola 's comments, we will get this merged.

ruaridhw · 2020-03-12T09:17:06Z

@michaelbynum, @jsiirola, back to you.

jsiirola

I found some more questions (all in the testing). If you have a few minutes, it would be great if you could clean the testing up a little more. But, I am not going to hold up the PR for it. @michaelbynum?

jsiirola · 2020-03-12T13:41:02Z

pyomo/solvers/tests/checks/test_cplex.py

+
+    def get_mock_model(self):
+        model = ConcreteModel()
+        model.x = Var(within=Binary)


I missed this, but we should add 2 more things:

an indexed variable m.y = Var([1,2]) to this test

a new test that uses a copy of this model using the kernel API (I was going to suggest a change in the actual implementation before I realized it was likely to break things for Kernel. It would be very good to test that here)

an indexed variable m.y = Var([1,2]) to this test

Thanks, added.

Yeah, it sure didn't work for kernel 🤦‍♂

I'm not too familiar with that API so I've had to add special checks as to whether the variable is indexed (see var_is_indexed() var_iter()). Any alternatives to make that neater would be great!

jsiirola · 2020-03-12T14:38:15Z

pyomo/solvers/tests/checks/test_cplex.py

+            "* ENCODING=ISO-8859-1\nNAME             Priority Order\n %s x1 %d\nENDATA\n"
+            % (ORDFileSchema._direction_to_str(direction_val), priority_val),


Below got updated, but I was thinking something closer to this here:

self.assertEqual( priorities_file, "* ENCODING=ISO-8859-1\n" "NAME Priority Order\n" " DN x1 10\n" "ENDATA\n")

jsiirola · 2020-03-12T14:39:48Z

pyomo/solvers/tests/checks/test_cplex.py

+        )
+
+
+class CPLEXShellSolvePrioritiesFile(unittest.TestCase):


Why is this a separate class from the previous priorities tester?

The former class is a unit test specifically on the writing of priorities via _write_priorities_file(). The latter is an integration test on the end-to-end application of priorities via the Suffix through a solve(). I've added docs to that effect

jsiirola

Overall, this is looking very good. Thank you!

I think I have one last suggestion for cleaning up the Var/Kernel iteration. If that works, then we should merge.

jsiirola · 2020-03-13T16:23:38Z

pyomo/solvers/plugins/solvers/CPLEX.py

@@ -268,14 +275,14 @@ def _convert_priorities_to_rows(self, instance, priorities, directions):

            var_direction = directions.get(var, BranchDirection.default)

-            if not var.is_indexed():


I think the easiest general solution is:

if hasattr(var, 'values'): # catches Var, IndexedVar, and kernel's variable_dict var_iter = var.values() elif hasattr(var, '__iter__'): # catches list and kernel's variable_list and variable_tuple var_iter = iter(var) else: # kernel's variable var_iter = iter((var,)) for v in var_iter: if id(v) not in byObject: continue v_direction = directions.get(v, var_direction) rows.append((byObject[id(v)], priority, v_direction))

Thanks @jsiirola! I've added this logic as a general-purpose helper in pyomo.util.components however let me know if you'd rather this logic is kept private to this method or this isn't the best place for it. (To be honest, I'm surprised this hasn't come up before!)

- Add iterator for looping over the data objects of either a `base` or `kernel` component

michaelbynum · 2020-03-16T12:26:55Z

OSX builds are failing, but it does not have anything to do with this PR. I just created another PR which will hopefully fix the problem. Then, we can get this merged.

michaelbynum · 2020-03-16T12:31:27Z

pyomo/util/components.py

+def iter_component(obj):
+    """
+    Yield "child" objects from a component that is defined with either the `base` or `kernel` APIs.
+    If the component is not indexed, it returns itself.
+
+    Parameters
+    ----------
+    obj : ComponentType
+        eg. `TupleContainer`, `ListContainer`, `DictContainer`, `IndexedComponent`, or `Component`
+
+    Returns
+    -------
+    Iterator[ComponentType] : Iterator of the component data objects.
+    """
+    try:
+        # catches `IndexedComponent`, and kernel's `_dict`
+        return iter(obj.values())
+    except AttributeError:
+        pass
+
+    try:
+        # catches list and kernel's `_list` and `_tuple`
+        return iter(obj)
+    except TypeError:
+        return iter((obj,))


@ruaridhw Thanks for adding this. This will be very useful in other places too.

michaelbynum

Everything looks good to me. Like I said, I am working on getting the osx builds fixed.

jsiirola

One fix for Py2/3 compatibility and we will merge.

jsiirola · 2020-03-16T17:59:56Z

pyomo/util/tests/test_components.py

@@ -7,10 +7,13 @@
 #  rights in this software.
 #  This software is distributed under the 3-clause BSD License.
 #  ___________________________________________________________________________
+from itertools import zip_longest


The import error can be fixed by:

from six.moves import zip_longest

Thanks, updated.

ruaridhw force-pushed the cplex_branching_priorities branch from 242ff65 to 9bbba62 Compare February 20, 2020 11:31

ruaridhw added 3 commits February 20, 2020 14:17

🚨 Add tests of solving with priorities

81f0a6e

- End-to-end tests of `opt.solve()` using branching priorities with CPLEXSHELL

🔨 Use Suffixes instead of _GeneralVarData

5e44956

- Declare `priority` and `direction` suffixes on the model instead of attributes on the variable data in order to represent branching decisions

ruaridhw force-pushed the cplex_branching_priorities branch from 9bbba62 to 5e44956 Compare February 20, 2020 15:24

ruaridhw mentioned this pull request Feb 20, 2020

Issue Branching Priorities #1299

Closed

ruaridhw added 4 commits February 20, 2020 15:32

🔨 Allow indexed variables to have separate directions

c4b9c53

🐛 Handle both non-indexed vars in Suffix

b468b2b

🚨 Update tests to work when CPLEX is unavailable

495776d

🐛 Fix test

dbccdf2

ruaridhw added 3 commits February 21, 2020 10:02

🐛 Fix imports

ba38d43

- Looks like there's some circular imports failing specifically for Py2.7 - Follow the same (hacky) local import design chosen in 0935e75

🔨 Make BranchDirection global and simplify _write_priorities_file()

e4f43f0

- Update tests to avoid usage of "magic numbers" and reference `BranchDirection` where possible

🔨 Remove unnecessary CPLEXBranchDirection

ae59e09

🔨 Use TestCase assert methods

6576269

blnicho requested review from ghackebeil, jsiirola and michaelbynum February 24, 2020 14:57

blnicho added the AT: PRE-TEST INSPECTED label Feb 24, 2020

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Feb 24, 2020

michaelbynum reviewed Mar 4, 2020

View reviewed changes

michaelbynum mentioned this pull request Mar 4, 2020

Parse noSolution status from CPLEXShell and CPLEXDirect solvers correctly #1313

Closed

jsiirola reviewed Mar 4, 2020

View reviewed changes

ruaridhw added 2 commits March 12, 2020 09:15

🚨 Register factories using top-level import

3eac4cc

🚨 Hardcode string results

4152424

jsiirola reviewed Mar 12, 2020

View reviewed changes

ruaridhw added 3 commits March 13, 2020 09:09

📚 Formatting of test results

37d1c77

📚 Test docs

e6d9fcc

🐛 Fix branching priorities for Kernel

1a0f41b

jsiirola reviewed Mar 13, 2020

View reviewed changes

ruaridhw added 2 commits March 16, 2020 11:50

🔨 Add iter_component()

67bfca0

- Add iterator for looping over the data objects of either a `base` or `kernel` component

🔨 Use iter_component()

e663b14

ruaridhw requested a review from jsiirola March 16, 2020 11:53

michaelbynum reviewed Mar 16, 2020

View reviewed changes

michaelbynum approved these changes Mar 16, 2020

View reviewed changes

jsiirola reviewed Mar 16, 2020

View reviewed changes

🐛 Fix Py2 import error

456cb86

jsiirola added the AT: PRE-TEST INSPECTED label Mar 17, 2020

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Mar 17, 2020

jsiirola approved these changes Mar 17, 2020

View reviewed changes

jsiirola merged commit c5ca6e1 into Pyomo:master Mar 17, 2020

ruaridhw deleted the cplex_branching_priorities branch March 17, 2020 22:37

ZedongPeng mentioned this pull request Aug 10, 2020

Using branch priority in Pyomo #1587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add branching priorities to CPLEXSHELL #1300

Add branching priorities to CPLEXSHELL #1300

ruaridhw commented Feb 19, 2020

codecov bot commented Feb 21, 2020 •

edited

Loading

ruaridhw commented Feb 24, 2020

michaelbynum left a comment

jsiirola Mar 4, 2020

jsiirola Mar 4, 2020

ruaridhw Mar 12, 2020

jsiirola Mar 12, 2020

jsiirola Mar 4, 2020

jsiirola Mar 4, 2020

michaelbynum Mar 7, 2020

ruaridhw Mar 12, 2020

ruaridhw commented Mar 4, 2020

michaelbynum commented Mar 7, 2020

ruaridhw commented Mar 12, 2020 •

edited

Loading

jsiirola left a comment

jsiirola Mar 12, 2020

ruaridhw Mar 13, 2020

jsiirola Mar 12, 2020

jsiirola Mar 12, 2020

ruaridhw Mar 13, 2020

jsiirola left a comment

jsiirola Mar 13, 2020

ruaridhw Mar 16, 2020

michaelbynum commented Mar 16, 2020

michaelbynum Mar 16, 2020

michaelbynum left a comment

jsiirola left a comment

jsiirola Mar 16, 2020

ruaridhw Mar 16, 2020

		from pyomo.solvers.plugins.converter.model import PyomoMIPConverter # register the `ProblemConverterFactory`
		from pyomo.repn.plugins.cpxlp import ProblemWriter_cpxlp # register the `WriterFactory`

		"* ENCODING=ISO-8859-1\nNAME Priority Order\n %s x1 %d\nENDATA\n"
		% (ORDFileSchema._direction_to_str(direction_val), priority_val),

		"* ENCODING=ISO-8859-1\nNAME Priority Order\n x1 %d\nENDATA\n"
		% (priority_val,),

		@@ -268,14 +275,14 @@ def _convert_priorities_to_rows(self, instance, priorities, directions):

		var_direction = directions.get(var, BranchDirection.default)

		if not var.is_indexed():

Add branching priorities to CPLEXSHELL #1300

Add branching priorities to CPLEXSHELL #1300

Conversation

ruaridhw commented Feb 19, 2020

Fixes #1299

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Feb 21, 2020 • edited Loading

Codecov Report

ruaridhw commented Feb 24, 2020

michaelbynum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruaridhw commented Mar 4, 2020

michaelbynum commented Mar 7, 2020

ruaridhw commented Mar 12, 2020 • edited Loading

jsiirola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsiirola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbynum commented Mar 16, 2020

Choose a reason for hiding this comment

michaelbynum left a comment

Choose a reason for hiding this comment

jsiirola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2020 •

edited

Loading

ruaridhw commented Mar 12, 2020 •

edited

Loading