Skip to content

Fix GDPopt LBB time-limit results - #3942

Closed
bernalde wants to merge 2 commits into
Pyomo:mainfrom
SECQUOIA:fix/issue-3941-gdpopt-lbb-time-limit
Closed

Fix GDPopt LBB time-limit results#3942
bernalde wants to merge 2 commits into
Pyomo:mainfrom
SECQUOIA:fix/issue-3941-gdpopt-lbb-time-limit

Conversation

@bernalde

Copy link
Copy Markdown
Contributor

Summary

  • Replace GDPopt LBB's stale _get_final_results_object() call on the time-limit path with _get_final_pyomo_results_object().
  • Add a solver-independent LBB regression test that forces the time-limit branch and verifies maxTimeLimit results.
  • Add MindtPy GOA time-limit coverage to verify its inherited GOA path sets maxTimeLimit on SolverResults.

Tests run

  • pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py::TestGDPopt_LBB_TimeLimit::test_time_limit_returns_pyomo_results_object pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py::TestMindtPyGOATimeLimit::test_goa_time_limit_sets_solver_results_condition - passed, 2 passed in 0.52s
  • pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 32 passed, 9 skipped, 2 deselected in 1.86s
  • python -m black --check --diff pyomo/contrib/gdpopt/branch_and_bound.py pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 3 files would be left unchanged
  • git diff --check -- pyomo/contrib/gdpopt/branch_and_bound.py pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed

Notes about tests not run

  • Full repository pytest and full-repository Black were not run locally because the targeted change is narrow and the full CI matrix covers multiple Python versions, platforms, optional solver installs, documentation, and coverage jobs.

Closes #3941

@bernalde bernalde left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

  1. Blocking issues: none.
  2. Nonblocking issues: none.
  3. Questions: none.
  4. Tests run:
    • pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py::TestGDPopt_LBB_TimeLimit::test_time_limit_returns_pyomo_results_object pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py::TestMindtPyGOATimeLimit::test_goa_time_limit_sets_solver_results_condition - passed, 2 passed in 0.50s.
    • pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 32 passed, 9 skipped, 2 deselected in 1.69s.
    • python -m black --check --diff pyomo/contrib/gdpopt/branch_and_bound.py pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 3 files unchanged. Black emitted its Python 3.13 / Python 3.14 parser warning but completed successfully.
    • git diff --check upstream/main...HEAD -- pyomo/contrib/gdpopt/branch_and_bound.py pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed.
    • gh pr checks 3942 --repo Pyomo/pyomo - currently pending overall. Passing at review time: Read the Docs, lint/style-and-typos, linux/3.10/bare-env, linux/3.10/slim, linux/3.11/mpi, linux/3.12/numpy2. Remaining GitHub Actions matrix jobs and Jenkins inspection are still pending.
  5. Merge as-is: I do not see code changes needed from review, but I would wait for the pending required CI/Jenkins statuses before merging.

@bernalde

Copy link
Copy Markdown
Contributor Author

Coordination note for #3946:

This PR and #3946 fix different bugs. This one fixes the LBB time-limit result path for #3941; #3946 fixes continuous-node solver dispatch for #3945. #3946 does not supersede this PR.

I checked the combined patches with git merge-tree. The runtime changes are independent, but both PRs add imports/tests near the top of pyomo/contrib/gdpopt/tests/test_LBB.py, so the second PR merged will likely need a small conflict resolution there.

Recommendation: merge this PR first since it is smaller and already through the GitHub Actions matrix, then refresh/rebase #3946 afterward.

@emma58 emma58 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long run we should think about moving the GDPopt solvers onto the new contrib.solver results object, but this looks good for now--it fixes a dumb bug in LBB leftover from a long ago GDPopt rewrite.


class TestMindtPyGOATimeLimit(unittest.TestCase):
def test_goa_time_limit_sets_solver_results_condition(self):
from pyomo.contrib.mindtpy.global_outer_approximation import MindtPy_GOA_Solver

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the import to the top of the file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ede4b3787 by moving MindtPy_GOA_Solver to the file-level imports in pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py. The GOA regression still passes with pytest -q pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py::TestMindtPyGOATimeLimit::test_goa_time_limit_sets_solver_results_condition.

@bernalde

Copy link
Copy Markdown
Contributor Author

Review comments addressed.

Commits pushed:

  • ede4b3787 - Address MindtPy GOA test import review

Main changes made:

  • Moved MindtPy_GOA_Solver import from inside test_goa_time_limit_sets_solver_results_condition() to the top-level import block in pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py.

Tests run:

  • pytest -q pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py::TestMindtPyGOATimeLimit::test_goa_time_limit_sets_solver_results_condition - passed, 1 passed in 0.79s.
  • pytest -q pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 29 passed in 1.59s.
  • pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py::TestGDPopt_LBB_TimeLimit::test_time_limit_returns_pyomo_results_object pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py::TestMindtPyGOATimeLimit::test_goa_time_limit_sets_solver_results_condition - passed, 2 passed in 0.57s.
  • python -m black --check --diff pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py - passed, 1 file unchanged. Black emitted its Python 3.13 / Python 3.14 parser warning but completed successfully.

Comments intentionally not addressed:

Remaining risks or follow-up items:

@mrmundt

mrmundt commented May 12, 2026

Copy link
Copy Markdown
Contributor

@bernalde - Required legal acknowledgement was deleted and tests are failing. Please feel free to reopen with the correct template once tests are passing.

@bernalde

Copy link
Copy Markdown
Contributor Author

Replaced by #4010.

This PR was closed because its description did not follow the repository pull request template — the AI-Use Disclosure and Legal Acknowledgement sections were absent. GitHub would not allow it to be reopened, so the work continues in a new PR.

#4010 carries over the implementation and tests from here unchanged, rebased onto current main, with a description built from .github/PULL_REQUEST_TEMPLATE.md with all sections intact.

For reference, the fix is still needed: pyomo/contrib/gdpopt/branch_and_bound.py on current main still calls the stale _get_final_results_object() on the LBB time-limit path, while the other termination site in the same file already uses _get_final_pyomo_results_object(). Issue #3941 remains open.

@emma58's approving review on this PR is the reason #4010 was prioritized first among the replacements. That approval does not transfer to the new PR, so #4010 needs a fresh review. The discussion here remains the historical record; this branch was left untouched so the diff stays readable.

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

Successfully merging this pull request may close these issues.

GDPopt LBB time-limit path calls stale _get_final_results_object

4 participants