From d11cfb127cb941ddf6b02aff4d39e622bbacebde Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Thu, 13 Jun 2024 22:17:12 -0600 Subject: [PATCH 1/9] unit tests for intermediate callback --- .../pynumero/interfaces/cyipopt_interface.py | 95 ++++++++++++++++--- .../tests/test_cyipopt_interface.py | 66 ++++++++++++- 2 files changed, 145 insertions(+), 16 deletions(-) diff --git a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py index 7845a4c189e..77c5f5db2fa 100644 --- a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py @@ -21,6 +21,7 @@ objects for the matrices (e.g., AmplNLP and PyomoNLP) """ import abc +import inspect from pyomo.common.dependencies import attempt_import, numpy as np, numpy_available from pyomo.contrib.pynumero.exceptions import PyNumeroEvaluationError @@ -309,6 +310,38 @@ def __init__(self, nlp, intermediate_callback=None, halt_on_evaluation_error=Non # cyipopt.Problem.__init__ super(CyIpoptNLP, self).__init__() + self._use_13arg_callback = None + if self._intermediate_callback is not None: + signature = inspect.signature(self._intermediate_callback) + positional_kinds = { + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.POSITIONAL_ONLY, + } + positional = [ + param + for param in signature.parameters.values() + if param.kind in positional_kinds + ] + has_var_args = any( + p.kind is inspect.Parameter.VAR_POSITIONAL + for p in signature.parameters.values() + ) + + if len(positional) == 13 and not has_var_args: + # If *args is expected, we do not use the new callback + # signature. + self._use_13arg_callback = True + elif len(positional) == 12 or has_var_args: + # If *args is expected, we use the old callback signature + # for backwards compatibility. + self._use_13arg_callback = False + else: + raise ValueError( + "Invalid intermediate callback. A function with either 12" + "or 13 positional arguments, or a variable number of arguments," + "is expected." + ) + def _set_primals_if_necessary(self, x): if not np.array_equal(x, self._cached_x): self._nlp.set_primals(x) @@ -436,19 +469,53 @@ def intermediate( alpha_pr, ls_trials, ): + """Calls user's intermediate callback + + This method has the call signature expected by CyIpopt. We then extend + this call signature to provide users of this interface class additional + functionality. Additional arguments are: + + - The ``NLP`` object that was used to construct this class instance. + This is useful for querying the variables, constraints, and + derivatives during the callback. + - The class instance itself. This is useful for calling the + ``get_current_iterate`` and ``get_current_violations`` methods, which + query Ipopt's internal data structures to provide this information. + + """ if self._intermediate_callback is not None: - return self._intermediate_callback( - self._nlp, - alg_mod, - iter_count, - obj_value, - inf_pr, - inf_du, - mu, - d_norm, - regularization_size, - alpha_du, - alpha_pr, - ls_trials, - ) + if self._use_13arg_callback: + # This is the callback signature expected as of Pyomo vTBD + return self._intermediate_callback( + self._nlp, + self, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ) + else: + # This is the callback signature expected pre-Pyomo vTBD and + # is supported for backwards compatibility. + return self._intermediate_callback( + self._nlp, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ) return True diff --git a/pyomo/contrib/pynumero/interfaces/tests/test_cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/tests/test_cyipopt_interface.py index bbcd6d4f26d..b8cfa4058bf 100644 --- a/pyomo/contrib/pynumero/interfaces/tests/test_cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/tests/test_cyipopt_interface.py @@ -96,13 +96,17 @@ def hessian(self, x, y, obj_factor): problem.solve(x0) -def _get_model_nlp_interface(halt_on_evaluation_error=None): +def _get_model_nlp_interface(halt_on_evaluation_error=None, intermediate_callback=None): m = pyo.ConcreteModel() m.x = pyo.Var([1, 2, 3], initialize=1.0) m.obj = pyo.Objective(expr=m.x[1] * pyo.sqrt(m.x[2]) + m.x[1] * m.x[3]) m.eq1 = pyo.Constraint(expr=m.x[1] * pyo.sqrt(m.x[2]) == 1.0) nlp = PyomoNLP(m) - interface = CyIpoptNLP(nlp, halt_on_evaluation_error=halt_on_evaluation_error) + interface = CyIpoptNLP( + nlp, + halt_on_evaluation_error=halt_on_evaluation_error, + intermediate_callback=intermediate_callback, + ) bad_primals = np.array([1.0, -2.0, 3.0]) indices = nlp.get_primal_indices([m.x[1], m.x[2], m.x[3]]) bad_primals = bad_primals[indices] @@ -219,6 +223,64 @@ def test_error_in_hessian_halt(self): with self.assertRaisesRegex(PyNumeroEvaluationError, msg): interface.hessian(bad_x, [1.0], 0.0) + def test_intermediate_12arg(self): + iterate_data = [] + + def intermediate( + nlp, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ): + self.assertIsInstance(nlp, PyomoNLP) + iterate_data.append((inf_pr, inf_du)) + + m, nlp, interface, bad_x = _get_model_nlp_interface( + intermediate_callback=intermediate + ) + # The interface's callback is always called with 11 arguments (by CyIpopt/Ipopt) + # but we add the NLP object to the arguments. + interface.intermediate(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) + self.assertEqual(iterate_data, [(4, 5)]) + + def test_intermediate_13arg(self): + iterate_data = [] + + def intermediate( + nlp, + problem, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ): + self.assertIsInstance(nlp, PyomoNLP) + self.assertIsInstance(problem, cyipopt.Problem) + iterate_data.append((inf_pr, inf_du)) + + m, nlp, interface, bad_x = _get_model_nlp_interface( + intermediate_callback=intermediate + ) + # The interface's callback is always called with 11 arguments (by CyIpopt/Ipopt) + # but we add the NLP object *and the cyipopt.Problem object* to the arguments. + interface.intermediate(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) + self.assertEqual(iterate_data, [(4, 5)]) + if __name__ == "__main__": unittest.main() From fd039f5dacffe535dfd97b268ff250d4ab65e82f Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Thu, 13 Jun 2024 22:43:18 -0600 Subject: [PATCH 2/9] add integration tests for cyipopt intermediate callback with 12 or 13 args --- .../solvers/tests/test_cyipopt_solver.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py index 0af5a772c98..d2fc09ddb15 100644 --- a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py +++ b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py @@ -326,3 +326,86 @@ def test_solve_without_objective(self): res = solver.solve(m, tee=True) pyo.assert_optimal_termination(res) self.assertAlmostEqual(m.x[1].value, 9.0) + + def test_solve_13arg_callback(self): + m = create_model1() + + iterate_data = [] + def intermediate( + nlp, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ): + x = nlp.get_primals() + y = nlp.get_duals() + iterate_data.append((x, y)) + + x_sol = np.array([3.85958688, 4.67936007, 3.10358931]) + y_sol = np.array([-1.0, 53.90357665]) + + solver = pyo.SolverFactory("cyipopt", intermediate_callback=intermediate) + res = solver.solve(m, tee=True) + pyo.assert_optimal_termination(res) + + # Make sure iterate vectors have the right shape and that the final + # iterate contains the primal solution we expect. + for x, y in iterate_data: + self.assertEqual(x.shape, (3,)) + self.assertEqual(y.shape, (2,)) + x, y = iterate_data[-1] + self.assertTrue(np.allclose(x_sol, x)) + # Note that we can't assert that dual variables in the NLP are those + # at the solution because, at this point in the algorithm, the NLP + # only has access to the *previous iteration's* dual values. + + # The 13-arg callback works with cyipopt < 1.3, but we will use the + # get_current_iterate method, which is only available in 1.3+ + @unittest.skipIf(not cyipopt_ge_1_3, "cyipopt version < 1.3.0") + def test_solve_13arg_callback(self): + m = create_model1() + + iterate_data = [] + def intermediate( + nlp, + problem, + alg_mod, + iter_count, + obj_value, + inf_pr, + inf_du, + mu, + d_norm, + regularization_size, + alpha_du, + alpha_pr, + ls_trials, + ): + iterate = problem.get_current_iterate() + x = iterate["x"] + y = iterate["mult_g"] + iterate_data.append((x, y)) + + x_sol = np.array([3.85958688, 4.67936007, 3.10358931]) + y_sol = np.array([-1.0, 53.90357665]) + + solver = pyo.SolverFactory("cyipopt", intermediate_callback=intermediate) + res = solver.solve(m, tee=True) + pyo.assert_optimal_termination(res) + + # Make sure iterate vectors have the right shape and that the final + # iterate contains the primal and dual solution we expect. + for x, y in iterate_data: + self.assertEqual(x.shape, (3,)) + self.assertEqual(y.shape, (2,)) + x, y = iterate_data[-1] + self.assertTrue(np.allclose(x_sol, x)) + self.assertTrue(np.allclose(y_sol, y)) From b1cc0a494bf8075f7cd86e4c6063305868d2f7f2 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Thu, 13 Jun 2024 22:45:52 -0600 Subject: [PATCH 3/9] apply black --- .../pynumero/algorithms/solvers/tests/test_cyipopt_solver.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py index d2fc09ddb15..b362e3ffca0 100644 --- a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py +++ b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py @@ -331,6 +331,7 @@ def test_solve_13arg_callback(self): m = create_model1() iterate_data = [] + def intermediate( nlp, alg_mod, @@ -374,6 +375,7 @@ def test_solve_13arg_callback(self): m = create_model1() iterate_data = [] + def intermediate( nlp, problem, From 170df3e9085478b7cba6bad52acebc6229bc8d9a Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Fri, 14 Jun 2024 16:51:26 -0600 Subject: [PATCH 4/9] guard against cyipopt not avilable --- .../pynumero/algorithms/solvers/tests/test_cyipopt_solver.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py index b362e3ffca0..60e3a72245e 100644 --- a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py +++ b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py @@ -370,7 +370,9 @@ def intermediate( # The 13-arg callback works with cyipopt < 1.3, but we will use the # get_current_iterate method, which is only available in 1.3+ - @unittest.skipIf(not cyipopt_ge_1_3, "cyipopt version < 1.3.0") + @unittest.skipIf( + not cyipopt_available or not cyipopt_ge_1_3, "cyipopt version < 1.3.0" + ) def test_solve_13arg_callback(self): m = create_model1() From 8892c852f36e26cea83b7110a630f5190379c6c1 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 10 Jul 2024 15:34:19 -0600 Subject: [PATCH 5/9] replace vTBD with current dev version --- pyomo/contrib/pynumero/interfaces/cyipopt_interface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py index 77c5f5db2fa..0fdc4cfe19a 100644 --- a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py @@ -485,7 +485,7 @@ def intermediate( """ if self._intermediate_callback is not None: if self._use_13arg_callback: - # This is the callback signature expected as of Pyomo vTBD + # This is the callback signature expected as of Pyomo 6.7.4.dev0 return self._intermediate_callback( self._nlp, self, @@ -502,7 +502,7 @@ def intermediate( ls_trials, ) else: - # This is the callback signature expected pre-Pyomo vTBD and + # This is the callback signature expected pre-Pyomo 6.7.4.dev0 and # is supported for backwards compatibility. return self._intermediate_callback( self._nlp, From 2b8f4e09928d294f267b97cc31f0f182d9ba80d5 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 10 Jul 2024 15:35:35 -0600 Subject: [PATCH 6/9] spaces in error message --- pyomo/contrib/pynumero/interfaces/cyipopt_interface.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py index 0fdc4cfe19a..183249b3793 100644 --- a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py @@ -337,9 +337,9 @@ def __init__(self, nlp, intermediate_callback=None, halt_on_evaluation_error=Non self._use_13arg_callback = False else: raise ValueError( - "Invalid intermediate callback. A function with either 12" - "or 13 positional arguments, or a variable number of arguments," - "is expected." + "Invalid intermediate callback. A function with either 12 or 13" + " positional arguments, or a variable number of arguments, is" + " expected." ) def _set_primals_if_necessary(self, x): From 142341f32a5851c5f3c9893bb165c2e71ebd99be Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 10 Jul 2024 15:49:37 -0600 Subject: [PATCH 7/9] add comment attempting to explain myself --- .../contrib/pynumero/interfaces/cyipopt_interface.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py index 183249b3793..26b43c37d1c 100644 --- a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py @@ -310,6 +310,17 @@ def __init__(self, nlp, intermediate_callback=None, halt_on_evaluation_error=Non # cyipopt.Problem.__init__ super(CyIpoptNLP, self).__init__() + # Pre-Pyomo 6.7.4.dev0, we had no way to pass the cyipopt.Problem object + # to the user in an intermediate callback. This prevented them from calling + # the useful get_current_iterate and get_current_violations methods. Now, + # we support this by adding the Problem object to the args we pass to a user's + # callback. To preserve backwards compatibility, we inspect the user's + # callback to infer whether they want this argument. To preserve backwards + # if the user asked for variable-length *args, we only pass the Problem as + # an argument if their callback asks for exactly 13 arguments. + # A more maintainable solution may be to force users to accept **kwds if they + # want "extra info." If we find ourselves continuing to augment this callback, + # this may be worth considering. -RBP self._use_13arg_callback = None if self._intermediate_callback is not None: signature = inspect.signature(self._intermediate_callback) From c9956acc760404defee1182ef6ade8c5c826d389 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Fri, 26 Jul 2024 16:48:05 -0600 Subject: [PATCH 8/9] grammar --- pyomo/contrib/pynumero/interfaces/cyipopt_interface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py index 26b43c37d1c..5187efadac9 100644 --- a/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py +++ b/pyomo/contrib/pynumero/interfaces/cyipopt_interface.py @@ -316,8 +316,8 @@ def __init__(self, nlp, intermediate_callback=None, halt_on_evaluation_error=Non # we support this by adding the Problem object to the args we pass to a user's # callback. To preserve backwards compatibility, we inspect the user's # callback to infer whether they want this argument. To preserve backwards - # if the user asked for variable-length *args, we only pass the Problem as - # an argument if their callback asks for exactly 13 arguments. + # compatibility if the user asked for variable-length *args, we do not pass + # the Problem object as an argument in this case. # A more maintainable solution may be to force users to accept **kwds if they # want "extra info." If we find ourselves continuing to augment this callback, # this may be worth considering. -RBP From 58b5df8d74fa254e152adf1ef4b0b8515bb0e04d Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Tue, 13 Aug 2024 10:04:00 -0600 Subject: [PATCH 9/9] test ipopt version before trying get_current_iterate --- .../algorithms/solvers/tests/test_cyipopt_solver.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py index 60e3a72245e..8a35449d94d 100644 --- a/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py +++ b/pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_solver.py @@ -46,6 +46,7 @@ # We don't raise unittest.SkipTest if not cyipopt_available as there is a # test below that tests an exception when cyipopt is unavailable. cyipopt_ge_1_3 = hasattr(cyipopt, "CyIpoptEvaluationError") + ipopt_ge_3_14 = cyipopt.IPOPT_VERSION >= (3, 14, 0) def create_model1(): @@ -369,11 +370,12 @@ def intermediate( # only has access to the *previous iteration's* dual values. # The 13-arg callback works with cyipopt < 1.3, but we will use the - # get_current_iterate method, which is only available in 1.3+ + # get_current_iterate method, which is only available in 1.3+ and IPOPT 3.14+ @unittest.skipIf( - not cyipopt_available or not cyipopt_ge_1_3, "cyipopt version < 1.3.0" + not cyipopt_available or not cyipopt_ge_1_3 or not ipopt_ge_3_14, + "cyipopt version < 1.3.0", ) - def test_solve_13arg_callback(self): + def test_solve_get_current_iterate(self): m = create_model1() iterate_data = []