Refactor CyIpopt interface to subclass `cyipopt.Problem` #2760

Robbybp · 2023-03-02T03:29:22Z

Summary/Motivation:

In mechmotum/cyipopt#182, I am working on an interface to let us call GetIpoptCurrentIterate and GetIpoptCurrentViolations during an intermediate callback. The non-hack way to do this without augmenting CyIpopt's callback signature is to add methods on cyipopt.Problem. This means that our callback in Pyomo will need to know about the cyipopt.Problem object, which can be achieved by having our "interface object" subclass cyipopt.Problem rather than passing our interface object to cyipopt.Problem when it is constructed.

This is a very small change in terms of lines of code, as cyipopt.Problem will automatically look for methods on self, so we don't need a new wrapper class, if it is not given a problem_obj (our CyIpoptProblemInterface) during construction.

Accidentally built this on top of #2759...

Changes

Update CyIpoptProblemInterface to subclass cyipopt.Problem
Call super().__init__ to initialize the cyipopt.Problem (if we forget this on any derived class, we segfault!)
Use the CyIpoptProblemInterface instead of creating a new cyipopt.Problem during solve methods

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:

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.

…s an argument

Robbybp · 2023-03-02T03:38:55Z

pyomo/contrib/pynumero/algorithms/solvers/cyipopt_solver.py

@@ -148,7 +148,7 @@ def _cyipopt_importer():
    'Internal_Error': TerminationCondition.internalSolverError,
 }

-class CyIpoptProblemInterface(object, metaclass=abc.ABCMeta):
+class CyIpoptProblemInterface(cyipopt.Problem, metaclass=abc.ABCMeta):


This fails if cyipopt is not available. We could get around this with

cyipopt_Problem = cyipopt.Problem if cyipopt_available else object

at the top of the file, but then we get a confusing error message when we try to call CyIpoptProblemInterface.solve. Maybe do this but raise an error in __init_subclass__ if we're not an instance of cyipopt.Problem?

What about using cyipopt_Problem as you suggest, then raising a sensible exception in __init__ if not cyipopt_available?

Not bad, we just have to remember to call the right combination of super().__init__s in subclasses. But I could move the cyipopt.Problem.__init__ call into CyIpoptProblemInterface.__init__, so subclass developers only have to remember to call one super().__init__.

…nto cyipopt-subclass

Robbybp · 2023-03-03T15:14:13Z

Test failures (on windows and linux/conda) are 'cyipopt' : Unexpected module found in 5 slowest-loading TPL modules, so I added cyipopt to the list of potentially slow-loading modules in test_tpl_import_time. Now, locally, I get failures like this:

    self.assertLess(tpl_time / total, 0.65)
AssertionError: 0.7089268883802343 not less than 0.65

I'm a little hesitant to adjust this hand-coded tolerance when I don't know why import time has suddenly increased. Maybe

cyipopt_Problem = cyipopt.Problem if cyipopt_available else object

is taking a ton of time during import?

jsiirola · 2023-03-03T15:14:24Z

pyomo/environ/tests/test_environ.py

+        if cyipopt_available:
+            ref.add('cyipopt')


We don't want to add cyipopt to this list: this is indicating that cyipopt is being unconditionally imported by pyomo.environ. We should re-think the imports so that we don't have to update this list.

Is this because PyomoCyIpoptSolver is registered as a plugin so it can be accessed via pyo.SolverFactory("cyipopt")?

Yes: pyomo.environ imports pyomo.contrib.pynumero.plugins which imports pyomo.contrib.pynumero.algorithms.solers.cyipopt_solver

I tried using a deferred import (via attempt_import) for pyomo.contrib.pynumero.algorithms.solvers.cyipopt_solver in pynumero.plugins, but that didn't change anything, nor did moving from .algorithms.solvers.cyipopt_solver import PyomoCyIpoptSolver to within the load function of pynumero.plugins.

Do you have any ideas?

jsiirola · 2023-03-03T15:40:44Z

I'm a little hesitant to adjust this hand-coded tolerance when I don't know why import time has suddenly increased. Maybe
cyipopt_Problem = cyipopt.Problem if cyipopt_available else object
is taking a ton of time during import?

Yes - I think the thing that changed is that we now cast cyipopt_available to a bool at the module scope. Casting cyipopt_available to bool is one of the ways to trigger the deferred module import (along with accessing an attribute on the cyipopt "module").

jsiirola · 2023-03-13T15:43:54Z

OK - I think I know how to solve the chicken & egg issue here:

split the ProblemInterface classes into a separate module from the solver interfaces (maybe .pynumero.interfaces.cyipopt_problem?)

Then, the solver interface can attempt_import the problem interface module, e.g.:

 cyipopt_problem, _ = attempt_import('pyomo.contrib.pynumero.interfaces.cyipopt_problem')

then the PyomoCyIpoptSolver can just problem = cyipopt_problem.CyIpoptNLP(...

That should prevent importing cyipopt until the solver interface is actually instantiated (actually, it would be deferred all the way until the first solve()).

…bclass

…roduce a deferred import back into cyipopt_solver

Robbybp · 2023-03-14T02:20:46Z

split the ProblemInterface classes into a separate module from the solver interfaces (maybe .pynumero.interfaces.cyipopt_problem?)
Then, the solver interface can attempt_import the problem interface module, e.g.:
 cyipopt_problem, _ = attempt_import('pyomo.contrib.pynumero.interfaces.cyipopt_problem')
then the PyomoCyIpoptSolver can just problem = cyipopt_problem.CyIpoptNLP(...

Thanks for the suggestion, this works for me locally.

Robbybp · 2023-03-14T02:25:53Z

pyomo/contrib/pynumero/algorithms/solvers/cyipopt_solver.py

+# These attributes should no longer be imported from this module. These
+# deprecation paths provide a deferred import to these attributes so (a) they
+# can still be used until these paths are removed, and (b) the imports are not
+# triggered when this module is imported.
+relocated_module_attribute(
+    "cyipopt_available",
+    "pyomo.contrib.pynumero.interfaces.cyipopt_interface.cyipopt_available",
+    "6.5.1.dev0",
+)
+relocated_module_attribute(
+    "CyIpoptProblemInterface",
+    "pyomo.contrib.pynumero.interfaces.cyipopt_interface.CyIpoptProblemInterface",
+    "6.5.1.dev0",
+)
+relocated_module_attribute(
+    "CyIpoptNLP",
+    "pyomo.contrib.pynumero.interfaces.cyipopt_interface.CyIpoptNLP",
+    "6.5.1.dev0",
+)


I commonly import cyipopt_available from this module, so I added these relocation warnings so these imports will still be available. This seems not to increase import time, judging by test_environ.py still passing locally.

… of cyipopt_solver

…og file is closed

Robbybp · 2023-03-14T20:08:40Z

        solver = pyo.SolverFactory('cyipopt')
        solver.config.options = {'hessian_approximation':'limited-memory',
                                 'nlp_scaling_method': 'user-scaling',
                                 'output_file': '_cyipopt-external-greybox-scaling.log',
                                 'file_print_level':10,
                                 'max_iter': 0}
        status = solver.solve(m, tee=False)
    
        with open('_cyipopt-external-greybox-scaling.log', 'r') as fd:
            solver_trace = fd.read()
>       os.remove('_cyipopt-external-greybox-scaling.log')
E       PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: '_cyipopt-external-greybox-scaling.log'

@jsiirola Actually, these are the options sent to Ipopt, so this file is created by Ipopt and probably not closed until the IpoptProblem (or whatever the name is) C struct is freed, which is whenever CyIpoptNLP.__dealloc__ is called, which I assume is when it gets garbage collected. This may be sufficient reason to implement a workaround in CyIpopt to avoid needing to subclass Problem to access the get_current_iterate functionality.

I just pushed a commit with gc.collect() before these calls to os.remove to test this hypothesis.

jsiirola · 2023-03-17T20:41:02Z

@Robbybp, I think we can resolve the Windows test failures by calling cyipopt.Problem.close() before attempting to delete the log file that cyipopt generated. The catch is that the Problem is private from the point of view of the tests (i.e., _problem). We could just access the private attribute in the test (i.e., solver._problem.close()), or we could make a more public API for releasing the cyipopt object (possibilities include promoting close(), or making a context manager that calls close() on __exit__()).

Thoughts?

Robbybp · 2023-03-18T01:59:35Z

@jsiirola Just pushed a commit that calls close() before attempting to remove the file. If this works, I'll work on a public API.

codecov · 2023-03-21T18:12:32Z

Codecov Report

Patch coverage: 90.37% and project coverage change: +0.01 🎉

Comparison is base (2128df4) 87.02% compared to head (74aadec) 87.03%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2760      +/-   ##
==========================================
+ Coverage   87.02%   87.03%   +0.01%     
==========================================
  Files         763      764       +1     
  Lines       87246    87268      +22     
==========================================
+ Hits        75926    75956      +30     
+ Misses      11320    11312       -8

Flag	Coverage Δ
linux	`84.04% <90.37%> (+<0.01%)`	⬆️
osx	`73.56% <45.98%> (+0.01%)`	⬆️
other	`84.22% <90.37%> (+<0.01%)`	⬆️
win	`81.56% <90.37%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...o/contrib/pynumero/interfaces/cyipopt_interface.py	`88.81% <88.81%> (ø)`
...trib/pynumero/algorithms/solvers/cyipopt_solver.py	`80.23% <100.00%> (-3.16%)`	⬇️
.../pynumero/algorithms/solvers/implicit_functions.py	`96.55% <100.00%> (+0.01%)`	⬆️
...b/pynumero/algorithms/solvers/pyomo_ext_cyipopt.py	`89.75% <100.00%> (+0.06%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…bclass

…lem attribute

…the log file

Robbybp · 2023-03-28T22:00:56Z

Highlights:

Updated CyIpoptProblemInterface to subclass cyipopt.Problem
Added a CyIpoptProblemInterface.__init__ method that calls cyipopt.Problem.__init__
Split cyipopt_solver.py into two files so PyomoCyIpoptSolver can be imported (and registered) even if cyipopt is not available
Add calls to CyIpoptNLP.close in a few tests to force deallocation Ipopt data structures so that Ipopt's log file can be removed on windows

@carldlaird can you take a look at this?

…bclass conflicts were resolved by taking the cyipopt-subclass files and running black on them.

Robbybp · 2023-04-08T19:11:04Z

pyomo/contrib/pynumero/algorithms/solvers/pyomo_ext_cyipopt.py

+        # Call CyIpoptProblemInterface.__init__, which calls
+        # cyipopt.Problem.__init__
+        super(PyomoExternalCyIpoptProblem, self).__init__()
+


Note that this __init__ call, which wasn't previously necessary, is now necessary in subclasses of CyIpoptProblemInterface to make sure that the cyipopt.Problem get initialized so we don't segfault when we try to solve it later.

In this way, this PR could break some user code if they are subclassing CyIpoptProblemInterface.

I am thinking of adding an __initialized flag to CyIpoptProblemInterface.__init__ so we can raise a warning/error if solve is called when cyipopt.Problem hasn't been initialized.

Robbybp · 2023-04-08T19:13:44Z

pyomo/contrib/pynumero/algorithms/solvers/tests/test_pyomo_ext_cyipopt.py

        with open('_cyipopt-pyomo-ext-scaling.log', 'r') as fd:
            solver_trace = fd.read()
+        cyipopt_problem.close()
        os.remove('_cyipopt-pyomo-ext-scaling.log')


This call to cyipopt.Problem.close is now necessary on windows if we try to remove a file too early (I think before the garbage collector has deallocated the cyipopt.Problem, but my testing was inconclusive).

This is another way this PR could break a user's code.

pyomo/contrib/pynumero/algorithms/solvers/cyipopt_solver.py

…bclass

…init__ has not been called

Robbybp · 2023-04-22T20:35:54Z

I've added a _problem_initialized flag that we set in CyIpoptProblemInterface.__init__, then check in CyIpoptProblemInterface.solve. In this way, if somebody overrides __init__ but doesn't call super().__init__, they will get an error when we attempt to call solve in e.g. PyomoCyIpoptSolver. This should prevent segfaults in this case, unless the user has also overridden solve, in which case they are on their own.

subclass cyipopt.Problem instead of passing CyIpoptProblemInterface a…

2ef3754

…s an argument

Robbybp commented Mar 2, 2023

View reviewed changes

Robbybp and others added 7 commits March 2, 2023 19:40

call cyipopt.Problem.__init__ from CyIpoptProblemInterface.__init__

1622477

add test for error message when cyipopt not available

1196a5b

Merge branch 'main' into cyipopt-subclass

1509831

Merge branch 'cyipopt-subclass' of https://github.com/robbybp/pyomo i…

20b536e

…nto cyipopt-subclass

skip test that uses CyIpoptNLP unless cyipopt_available

fc1a533

add cyipopt to list of potentially slow-loading modules

0f4503f

do not import cyipopt_available out of pyomo.common.dependencies

436221f

jsiirola reviewed Mar 3, 2023

View reviewed changes

remove cyipopt from slow-loading module set

ae55bde

Robbybp added 3 commits March 13, 2023 19:10

Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-su…

42d4de4

…bclass

move CyIpoptNLP and CyIpoptProblemInterface to their own file and int…

60c567f

…roduce a deferred import back into cyipopt_solver

move cyipopt_interface.py to pynumero/interfaces/

67dc2f1

Robbybp commented Mar 14, 2023

View reviewed changes

Robbybp added 3 commits March 13, 2023 20:35

access cyipopt_available from cyipopt_interface module

f06409a

update imports to import CyIpoptNLP out of cyipopt_interfaces instead…

1c56631

… of cyipopt_solver

call gc.collect before removing log files to try to make sure ipopt l…

028fafe

…og file is closed

Robbybp mentioned this pull request Mar 17, 2023

Add get_current_iterate and get_current_violations methods to Problem class mechmotum/cyipopt#182

Merged

jsiirola and others added 2 commits March 17, 2023 19:08

Merge branch 'main' into cyipopt-subclass

c956c3a

call close before removing log file to attempt to remove lock

c584364

Robbybp added 2 commits March 17, 2023 21:52

add call to cyipopt.Problem.close at end of PyomoCyIpoptSolver.solve

be5b2b9

add a cyipopt.Problem.close call to a test that was missing it

f528f1c

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Mar 21, 2023

blnicho self-requested a review March 22, 2023 19:53

Robbybp added 3 commits March 28, 2023 15:42

Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-su…

2bab691

…bclass

call close() on user-created problem object rather than private _prob…

fad198e

…lem attribute

note that close must be called to force Ipopt to release its lock on …

e90e242

…the log file

Robbybp added 3 commits April 8, 2023 12:48

Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-su…

8a19dd6

…bclass conflicts were resolved by taking the cyipopt-subclass files and running black on them.

run black on files I missed

2a07e10

re-run black, now with --skip-string-normalization

7edf82d

Robbybp commented Apr 8, 2023

View reviewed changes

pyomo/contrib/pynumero/algorithms/solvers/cyipopt_solver.py Outdated Show resolved Hide resolved

Robbybp added 5 commits April 17, 2023 14:17

Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-su…

fb451d9

…bclass

update formatting for consistency with black

e028e1d

fix indentation

eba9b39

comma at end of arg list

20e6b1b

remove whitespace

5a8d7b2

jsiirola approved these changes Apr 18, 2023

View reviewed changes

jsiirola added the AT: PRE-TEST INSPECTED label Apr 18, 2023

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Apr 18, 2023

blnicho requested a review from carldlaird April 18, 2023 19:07

Robbybp added 2 commits April 22, 2023 13:22

Merge branch 'main' of https://github.com/pyomo/pyomo into cyipopt-su…

5dde096

…bclass

add flag and check to catch if solve is calledwhen cyipopt.Problem.__…

928250b

…init__ has not been called

NFC: Fixing string formatting

74aadec

blnicho approved these changes Apr 24, 2023

View reviewed changes

blnicho added the AT: PRE-TEST INSPECTED label Apr 24, 2023

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Apr 24, 2023

blnicho merged commit 9f57f07 into Pyomo:main Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CyIpopt interface to subclass `cyipopt.Problem` #2760

Refactor CyIpopt interface to subclass `cyipopt.Problem` #2760

Robbybp commented Mar 2, 2023

Robbybp Mar 2, 2023 •

edited

Loading

jsiirola Mar 2, 2023

Robbybp Mar 2, 2023

Robbybp commented Mar 3, 2023

jsiirola Mar 3, 2023

Robbybp Mar 3, 2023

jsiirola Mar 3, 2023

Robbybp Mar 3, 2023 •

edited

Loading

jsiirola commented Mar 3, 2023

jsiirola commented Mar 13, 2023

Robbybp commented Mar 14, 2023

Robbybp Mar 14, 2023

Robbybp commented Mar 14, 2023

jsiirola commented Mar 17, 2023

Robbybp commented Mar 18, 2023

codecov bot commented Mar 21, 2023 •

edited

Loading

Robbybp commented Mar 28, 2023

Robbybp Apr 8, 2023

Robbybp Apr 21, 2023

Robbybp Apr 8, 2023

Robbybp commented Apr 22, 2023

Refactor CyIpopt interface to subclass cyipopt.Problem #2760

Refactor CyIpopt interface to subclass cyipopt.Problem #2760

Conversation

Robbybp commented Mar 2, 2023

Summary/Motivation:

Changes

Legal Acknowledgement

Robbybp Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbybp commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbybp Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

jsiirola commented Mar 3, 2023

jsiirola commented Mar 13, 2023

Robbybp commented Mar 14, 2023

Choose a reason for hiding this comment

Robbybp commented Mar 14, 2023

jsiirola commented Mar 17, 2023

Robbybp commented Mar 18, 2023

codecov bot commented Mar 21, 2023 • edited Loading

Codecov Report

Robbybp commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbybp commented Apr 22, 2023

Refactor CyIpopt interface to subclass `cyipopt.Problem` #2760

Refactor CyIpopt interface to subclass `cyipopt.Problem` #2760

Robbybp Mar 2, 2023 •

edited

Loading

Robbybp Mar 3, 2023 •

edited

Loading

codecov bot commented Mar 21, 2023 •

edited

Loading