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

Add support for Xpress callbacks in Pyomo. #2648

Merged
merged 12 commits into from
Jan 31, 2023

Conversation

djunglas
Copy link
Contributor

@djunglas djunglas commented Dec 2, 2022

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

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.

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Base: 87.06% // Head: 87.07% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (863ff86) compared to base (5c4fee8).
Patch coverage: 80.14% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2648      +/-   ##
==========================================
+ Coverage   87.06%   87.07%   +0.01%     
==========================================
  Files         757      757              
  Lines       84511    85038     +527     
==========================================
+ Hits        73581    74051     +470     
- Misses      10930    10987      +57     
Flag Coverage Δ
linux 84.49% <80.14%> (-0.02%) ⬇️
osx 74.56% <80.14%> (+0.02%) ⬆️
other 84.67% <80.14%> (-0.02%) ⬇️
win 81.84% <80.14%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
pyomo/solvers/plugins/solvers/xpress_direct.py 75.24% <80.14%> (+2.41%) ⬆️
pyomo/contrib/parmest/parmest.py 88.47% <0.00%> (+4.29%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djunglas djunglas changed the title Add support for callbacks in Python. Add support for Xpress callbacks in Pyomo. Dec 7, 2022
@blnicho
Copy link
Member

blnicho commented Dec 19, 2022

@djunglas we typically don't review PRs until all the tests are passing. If you need help resolving the test failures let us know. I would also recommend merging the main branch into this PR since it is a couple weeks behind. If you check the option to grant repo maintainers write-access to this branch then we can help keep it up to date.

@djunglas
Copy link
Contributor Author

@djunglas we typically don't review PRs until all the tests are passing. If you need help resolving the test failures let us know. I would also recommend merging the main branch into this PR since it is a couple weeks behind. If you check the option to grant repo maintainers write-access to this branch then we can help keep it up to date.

Thanks for the heads up, @blnicho I did not notice the failures since locally all tests seemed to pass. I'll see to this after the Christmas break.

Nodes may be cutoff and thus the last call to the `optnode` callback may
not be for the last node processed.
@mrmundt
Copy link
Contributor

mrmundt commented Jan 10, 2023

@djunglas - Please merge main again. There was a workaround for the Windows failures that went in this morning.

@mrmundt mrmundt self-requested a review January 10, 2023 20:07
@djunglas
Copy link
Contributor Author

@djunglas - Please merge main again. There was a workaround for the Windows failures that went in this morning.

@mrmundt - I did that and now failures are down to problem with uploading artifacts from a Windows coverage run.

@mrmundt
Copy link
Contributor

mrmundt commented Jan 11, 2023

@mrmundt - I did that and now failures are down to problem with uploading artifacts from a Windows coverage run.

@djunglas - An aberrant issue. A re-run resolved that. I have inspected the code and have queued it for our internal test suite.

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

@djunglas - Overall, this looks great, and I thank you so much for submitting this PR. I found a few typos in documentation and have some comments about our preferred coding standard. I will not insist that these hold up the merging of the PR, however, since they are all NFC.

pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/plugins/solvers/xpress_direct.py Outdated Show resolved Hide resolved
pyomo/solvers/tests/checks/test_xpress_persistent.py Outdated Show resolved Hide resolved
pyomo/solvers/tests/checks/test_xpress_persistent.py Outdated Show resolved Hide resolved
@mrmundt
Copy link
Contributor

mrmundt commented Jan 20, 2023

@michaelbynum - Please review this.

@mrmundt
Copy link
Contributor

mrmundt commented Jan 31, 2023

Hi @djunglas - I inadvertently merged this today. We are very excited about this PR but need to have a more targeted discussion as a dev team to discuss backwards compatibility implications. Once we revert back, I will reopen this PR. Apologies for my mistake!

@mrmundt
Copy link
Contributor

mrmundt commented Jan 31, 2023

Second layer of apologies, @djunglas - GitHub will not allow me to reopen this PR. Sigh! Can you please open a new PR, which I will approve, and then we will get back to you ASAP after our dev team discussion?

@djunglas
Copy link
Contributor Author

djunglas commented Feb 1, 2023

Hi @mrmundt - I tried to create new PR but it won't let me do that since it claims there are no differences between by branch and pyomo/main now. Did you already revert things on main?
By the way, I don't see the backwards compat issue here. I designed the changes so that existing code would still work exactly the same way it did before.

@jsiirola
Copy link
Member

jsiirola commented Feb 1, 2023

It looks like your branch was updated from Pyomo/main after the PR was reverted. I think I can "reopen" this by reverting @mrmundt' revert of your PR. ... repoened as #2714

@jsiirola
Copy link
Member

jsiirola commented Feb 1, 2023

@djunglas: the (mostly my) concern is not with this PR breaking backwards compatibility, but rather when we merge this (since it is going into core functionality), we will be making a promise of maintaining backwards compatibility with this API moving forward. We are making a conscious effort to be intentional when adding / expanding / revising the core API that users interact with.

In this case, the discussion was motivated by both this PR and #2680, and touched on a couple points:

  • consistency with callback support being developed in APPSI (one of the intents behind APPSI is to form the base of all future solver interfaces)
  • where to draw a line between supporting a "consistent" (generic) solver-independent interface and where / how we should expose the raw solver interface (for use by advanced users)

This PR is in really good shape, and I expect that we will get it merged in (hopefully soon) -- unfortunately, some of the core Devs that we want to include in these discussions have been not been available this month, so it has taken longer to finish this. By the way, all are welcome to join the dev calls - email [email protected] if you would like the call-in information.

@djunglas
Copy link
Contributor Author

djunglas commented Feb 2, 2023

@jsiirola, thanks a lot for explaining your concerns. They make perfect sense to me. Not sure if this helps, but the callback API I am proposing here is not cast in stone. I would be willing to modify the callback support in case you want it to match some API you defined.

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.

5 participants