Skip to content

Fix GDPopt LBB time-limit results - #4010

Open
bernalde wants to merge 4 commits into
Pyomo:mainfrom
SECQUOIA:fix/issue-3941-lbb-time-limit-results
Open

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

Conversation

@bernalde

Copy link
Copy Markdown
Contributor

Fixes #3941.

Summary/Motivation:

The GDPopt LBB time-limit path calls self._get_final_results_object(), which no longer exists. The method was renamed to _get_final_pyomo_results_object() during an earlier GDPopt rewrite, and this one call site was left stale. Any LBB solve that terminates by hitting its time limit raises AttributeError instead of returning a results object.

The same file already uses the current name at the other termination site (branch_and_bound.py:288), so this is an isolated leftover rather than an intentional difference.

This is a replacement for #3942, which GitHub would not allow me to reopen after it was closed. That PR was approved by @emma58 and was closed for a PR-template/CI-state reason rather than a code concern; the prior review discussion and history remain available there.

Changes proposed in this PR:

  • Call _get_final_pyomo_results_object() on the GDPopt LBB time-limit termination path in pyomo/contrib/gdpopt/branch_and_bound.py, matching the other termination site in the same file.
  • Add TestGDPopt_LBB_TimeLimit.test_time_limit_returns_pyomo_results_object, a solver-independent regression that forces the time-limit branch and asserts a Pyomo results object is returned with maxTimeLimit and unbounded problem bounds.
  • Add TestMindtPyGOATimeLimit.test_goa_time_limit_sets_solver_results_condition, documenting that the corresponding MindtPy GOA time-limit path already sets its termination condition correctly.
  • Validation performed locally on this branch, rebased onto current main:
    • python -m pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py
      • Result: 32 passed, 9 skipped, 2 deselected in 2.55s
    • python -m pytest -q pyomo/contrib/gdpopt/tests/
      • Result: 76 passed, 35 skipped, 5 deselected in 81.25s
    • 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
      • Result: passed, 3 files would be left unchanged
    • typos --config ./.github/workflows/typos.toml on the three changed files
      • Result: passed, no findings

AI-Use Disclosure

  • AI tools were NOT used during the preparation of this PR

or

  • AI tools contributed to the development of this PR

    • AI tools generated documentation (including the PR description/comments, code comments, and/or Sphinx documentation)
    • AI tools generated tests (baselines, examples, and/or code)
    • AI tools generated code (apart from tests)

    Review process (select ONE):

    • Rewritten: All AI-generated content was rewritten by me before being committed.
    • Reviewed/verified: I retained AI-generated content and verified it before committing. Verification included (as applicable):
      • Ran the code and fixed issues
      • Added and ran tests
      • Checked correctness/logic of code and tests
      • Checked for alignment with the contribution guide
      • Considered security implications
    • As-is: AI-generated content was commited directly to the repository

Notes for reviewers (optional): This replacement PR carries over the implementation and tests from #3942 unchanged, then refreshes the branch against current main. The replacement PR description and branch-refresh workflow were prepared with AI assistance and reviewed before posting. The runtime change is a single call-site rename; reviewers may want to focus on whether the forced-time-limit regression test in test_LBB.py is the right way to exercise that path without requiring a MINLP subsolver, since it temporarily patches GDP_LBB_Solver.reached_time_limit and restores it in a finally block.

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:

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

@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.

One question about the LBB test, but otherwise this look good!

Comment thread pyomo/contrib/gdpopt/tests/test_LBB.py Outdated
Comment on lines +69 to +72
try:
results = SolverFactory('gdpopt.lbb').solve(m, time_limit=1, tee=False)
finally:
GDP_LBB_Solver.reached_time_limit = orig_reached_time_limit

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.

I don't understand the try-finally here: It looks like if line 70 throws an exception results will be undefined, which will lead to an exception in the assertions below.

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 b63db6190. If solve() raises, the original try/finally restores the method and then propagates that exception, so the assertions are not reached and results is not read. I replaced the manual assignment/restoration with patch.object(...) to make that lifecycle explicit; the focused regression test and full GDPopt suite pass.

@bernalde

Copy link
Copy Markdown
Contributor Author

Addressed the LBB test review in b63db6190.

  • Replaced manual GDP_LBB_Solver.reached_time_limit assignment/restoration with a scoped patch.object(...). If solve() raises, its exception propagates after cleanup and the assertions are not reached; the context manager makes that lifecycle explicit.
  • Local verification:
    • python -m pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py::TestGDPopt_LBB_TimeLimit::test_time_limit_returns_pyomo_results_object — 1 passed
    • python -m pytest -q pyomo/contrib/gdpopt/tests/test_LBB.py pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py — 32 passed, 9 skipped, 2 deselected
    • python -m pytest -q pyomo/contrib/gdpopt/tests/ — 73 passed, 38 skipped, 5 deselected
    • python -m black . --check --diff — passed
    • typos --config ./.github/workflows/typos.toml — passed
  • Current-head CI: GitHub Actions completed successfully across all 30 jobs, and Read the Docs succeeded.
  • Intentionally not addressed: none.
  • Remaining: Jenkins CI / inspection is still pending. The live review decision is REVIEW_REQUIRED; the review thread remains unresolved for the reviewer to close.

@bernalde

Copy link
Copy Markdown
Contributor Author

@emma58, the LBB test follow-up is addressed in b63db6190. Could you please take another look?

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

2 participants