Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Returning False from intermediate no longer halts ipopt #249

Closed
jsiirola opened this issue Apr 2, 2024 · 6 comments
Closed

Returning False from intermediate no longer halts ipopt #249

jsiirola opened this issue Apr 2, 2024 · 6 comments

Comments

@jsiirola
Copy link
Contributor

jsiirola commented Apr 2, 2024

Commit d4e986a (merged in #227, released in 1.4.0) changed the implementation of cdef Bool intermediate_cb() so that it no longer returns the result from the user-provided intermediate callback function. As a result, user-provided callbacks can no longer stop Ipopt by returning False.

@moorepants
Copy link
Collaborator

Would you have a simple test case for this?

@jsiirola
Copy link
Contributor Author

jsiirola commented Apr 2, 2024

Alas, not a simple test case - we saw it as a new failure in the Pyomo test suite (pyomo/contrib/pynumero/examples/tests/test_cyipopt_examples.py::TestExamples::test_cyipopt_callback_halt; e.g., https://github.com/Pyomo/pyomo/actions/runs/8514519228/job/23320399948?pr=3221).

I believe the fix is simply

diff --git a/cyipopt/cython/ipopt_wrapper.pyx b/cyipopt/cython/ipopt_wrapper.pyx
index 305b636..6b22953 100644
--- a/cyipopt/cython/ipopt_wrapper.pyx
+++ b/cyipopt/cython/ipopt_wrapper.pyx
@@ -1274,6 +1274,7 @@ cdef Bool intermediate_cb(Index alg_mod,

         if ret_val is None:
             return True
+        return ret_val
     except:
         self.__exception = sys.exc_info()
         return True

@moorepants
Copy link
Collaborator

I see the fix, but wanted to add a regression test.

@jsiirola
Copy link
Contributor Author

jsiirola commented Apr 2, 2024

Ahh - sorry - the test in question uses a model that is expected to take 8-10 iterations, adds an intermediate callback that is just:

def iteration_callback(
    nlp,
    alg_mod,
    iter_count,
    obj_value,
    inf_pr,
    inf_du,
    mu,
    d_norm,
    regularization_size,
    alpha_du,
    alpha_pr,
    ls_trials,
):
    if iter_count >= 4:
        return False
    return True

It then runs the solver and looks for the user interrupt return code. The problem is that it is all written in Pyomo (so leverages all the Pyomo modeling infrastructure and model conversions), so as written is probably a poor regression test for here.

@moorepants
Copy link
Collaborator

Thanks. I have a little test now.

moorepants added a commit that referenced this issue Apr 2, 2024
Fixed #249, ensure intermediate_cb returns its value if no exception.
@moorepants
Copy link
Collaborator

Fixed in version 1.4.1. Thanks for reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants