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

parmest with, or without, mpi-sppy #1778

Merged
merged 40 commits into from
Apr 30, 2021
Merged

Conversation

DLWoodruff
Copy link
Contributor

@DLWoodruff DLWoodruff commented Jan 7, 2021

Fixes # .

Summary/Motivation: This is a version of parmest that does not need PySP, but can use mpi-sppy if it is present.

Changes proposed in this PR:

  • drop use of PySP
  • add optional use of mpi-sppy
  • the bad news: use two files of code copied from mpi-sppy (this is legal, but less than ideal) so that we can avoid use of mpi-sppy but still have all the hooks
  • TBD: if we like this, we will want to add tests that do depend on the presence of mpi-sppy, which is easy to assure in the test environment (it is pip installable); however, for the time being, those tests should probably run in mpi-sppy.

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.

@jsiirola jsiirola mentioned this pull request Feb 16, 2021
@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

Merging #1778 (82538e0) into main (75439e9) will increase coverage by 1.17%.
The diff coverage is 81.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1778      +/-   ##
==========================================
+ Coverage   80.98%   82.15%   +1.17%     
==========================================
  Files         569      571       +2     
  Lines       71629    71699      +70     
==========================================
+ Hits        58008    58904     +896     
+ Misses      13621    12795     -826     
Impacted Files Coverage Δ
pyomo/contrib/parmest/parmest.py 77.77% <77.46%> (+66.49%) ⬆️
pyomo/contrib/parmest/scenario_tree.py 78.12% <78.12%> (ø)
pyomo/contrib/parmest/create_ef.py 84.16% <84.16%> (ø)
pyomo/contrib/mindtpy/iterate.py 80.74% <0.00%> (+2.48%) ⬆️
pyomo/opt/plugins/sol.py 85.87% <0.00%> (+2.82%) ⬆️
pyomo/contrib/parmest/mpi_utils.py 75.00% <0.00%> (+54.16%) ⬆️
.../parmest/examples/reactor_design/reactor_design.py 72.22% <0.00%> (+72.22%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75439e9...82538e0. Read the comment docs.

…y we really want to ship it (use_mpisppy=True, which means it will try to use mpi-sppy but fall back to local if mpi-sppy is not present)
@DLWoodruff DLWoodruff changed the title [WIP] parmest with, or without, mpi-sppy parmest with, or without, mpi-sppy Feb 21, 2021
@DLWoodruff DLWoodruff changed the title [WIP] parmest with, or without, mpi-sppy parmest with, or without, mpi-sppy Mar 29, 2021
@DLWoodruff DLWoodruff requested review from blnicho and kaklise March 31, 2021 17:09
Copy link
Member

@kaklise kaklise left a comment

Choose a reason for hiding this comment

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

I added minor comments to add the URL to mpi-sppy. Other than that, this looks good to me.

pyomo/contrib/parmest/scenario_tree.py Show resolved Hide resolved
pyomo/contrib/parmest/create_ef.py Outdated Show resolved Hide resolved
@blnicho blnicho dismissed their stale review April 20, 2021 04:52

Stale review

@DLWoodruff
Copy link
Contributor Author

Thanks for making the changes. The one test failure seems to be unrelated to parmest.

Copy link
Member

@blnicho blnicho left a comment

Choose a reason for hiding this comment

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

I have a few minor questions

pyomo/contrib/parmest/parmest.py Show resolved Hide resolved
Comment on lines +397 to +398
if (solver == "k_aug"):
raise RuntimeError("k_aug no longer supported.")
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add an issue to support k_aug in the future now that we have easier access to it through idaes-ext in our testing environment.

pyomo/contrib/parmest/parmest.py Show resolved Hide resolved
pyomo/contrib/parmest/parmest.py Show resolved Hide resolved
@DLWoodruff DLWoodruff changed the title parmest with, or without, mpi-sppy [WIP] parmest with, or without, mpi-sppy Apr 26, 2021
@DLWoodruff DLWoodruff changed the title [WIP] parmest with, or without, mpi-sppy parmest with, or without, mpi-sppy Apr 27, 2021
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