Add dense and sparse wrappers for ParOpt from ParOpt directly

[gjkennedy]

Purpose

This PR adds the ParOpt sparse and dense constraint wrappers for pyOptSparse from ParOpt directly.

Expected time until merged

Type of change

• [ ] Bugfix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (non-backwards-compatible fix or feature)
• [ ] Code style update (formatting, renaming)
• [ ] Refactoring (no functional changes, no API changes)
• [ ] Documentation update
• [ ] Maintenance update
• [ ] Other (please describe)

Testing

Checklist

• [ ] I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• [ ] I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• [ ] I have run unit and regression tests which pass locally with my changes
• [ ] I have added new tests that prove my fix is effective or that my feature works
• [ ] I have added necessary documentation

[A-CGray]

@marcomangano @ewu63 the actual pyoptsparse wrapper that this will end up using is here

[A-CGray]

The tests are failing because our docker images are pinned to an older version of ParOpt. But if we update the ParOpt version in the docker images before merging this PR then the image builds will fail and so the tests here will still fail. So I guess we need to test this PR locally, merge it even with failing azure tests, and then update the docker images. Opinions @ewu63 @eirikurj ?

[A-CGray]

@gjkennedy I've run the pyoptsparse tests locally, the dense version of your wrapper seems to work fine, but when using the sparse version the constraints seem to be ignored if you use the trust region algorithm which is the default, interior-point and mma work fine. Try running test_hs015.py

[marcomangano]

Specific test errors aside, I have mixed feelings about moving the actual pyOptSparse wrapper to the parOpt repo.

On the one hand, outsourcing it could make things a bit easier for us in terms of maintenance, especially if the SMDO group plans to update the python API on a regular basis. On the other hand, if we make breaking changes to our codebase, we would not be able to address it directly and have to wait for a new version release of ParOpt for our tests to pass. Since we run tests on our side, I am leaning towards moving the updated wrapper back here.

This would also avoid the kind of "circular import" happening now, where pyoptsparse imports paropt but the imported wrapper is also importing pyoptsparse components - but this is a minor note. Curious to hear @ewu63 and @eirikurj take on this.

[ewu63]

1. I have no idea how this circular dependency would be resolved, and in any case I don't think pyoptsparse-specific code should live in ParOpt. Instead, a generic Python interface should be provided. I think the correct approach would be the following, consistent with our other wrappers
   1. Keep the wrapper in this repo
   2. Test with specific tagged version, and request that changes to API be versioned appropriately
2. Once the wrapper is confirmed to be working, we can merge this PR and then update docker. The procedure you described is fine @A-CGray

[gjkennedy]

Hey guys, this is a PR. Feel free to accept or reject it. Do not feel free to copy code from the wrapper I wrote into your repo! Thanks!

[A-CGray]

@gjkennedy we can go with this approach, but there are currently some issues with the wrapper in the ParOpt. I opened https://github.com/smdogroup/paropt/pull/51 to fix them. With those fixes I can get the paropt tests in this repo to pass. Once that PR is merged and you make a new release of ParOpt, we can figure out how to get things working in our docker images.

@ewu63 , the MPI environment variable tests below are now failing because this PR removed this code, I had a look at adding it back but I can't understand the logic behind the code, could you take a look?

https://github.com/mdolab/pyoptsparse/blob/f66e9fbd6712dbb941794698cf87fc053c8a7986/tests/test_require_mpi_env_var.py#L45-L73

[A-CGray]

Marking this as a draft until https://github.com/smdogroup/paropt/pull/51 is fixed and a new ParOpt release is available

[A-CGray]

There is one more PR on ParOpt (https://github.com/smdogroup/paropt/pull/55) that needs to be merged to fix some small remaining issues and then the v2.1.5 release of ParOpt updated to the latest commit. Even once that is done, the tests here will continue to fail because the docker images use an old version of ParOpt. Previously we said we'd test this PR locally, merge it, then update the ParOpt version in the docker images.

I added a new set of tests that tests the three different ParOpt algorithms with both the dense and sparse versions of the wrapper on a constrained and unconstrained version of the Rosenbrock function. I also updated the tests related to the MPI environment variable.

[marcomangano]

This test is failing because the docker images are not updated to the latest paropt version, but I believe we could remove this test altogether at this point?

[A-CGray]

After discussion with @marcomangano and @ewu63, we decided that any testing of the ParOpt wrapper should be in the same repo as the wrapper itself. So I have removed the ParOpt optimiser tests from this repo, and opened https://github.com/smdogroup/paropt/pull/56 to move them to the ParOpt repo.

[A-CGray]

This test is failing because the docker images are not updated to the latest paropt version, but I believe we could remove this test altogether at this point?

I'm not sure, I think it depends on whether the OpenMDAO team (@robfalck) are still relying on this behaviour.

[robfalck]

Hey @A-CGray , I'm not entirely sure of the behavior we're checking for here, relative to OpenMDAO. If ParOpt tests are being moved out of ParOpt I think I'm ok with this test being removed.

[A-CGray]

Hey @A-CGray , I'm not entirely sure of the behavior we're checking for here, relative to OpenMDAO. If ParOpt tests are being moved out of ParOpt I think I'm ok with this test being removed.

Sorry, I wasn't very clear, specifically we're asking whether OpenMDAO still relies on the behaviour added by Herb in https://github.com/mdolab/pyoptsparse/pull/118 where pyOptSparse can be required to use or not use MPI using the PYOPTSPARSE_REQUIRE_MPI environment variable.

[robfalck]

Thanks for the reminder. We don't currently have any tests that set this variable...we only use OPENMDAO_USE_MPI for our purposes. I think we can skip or remove this for now. I like the ability to disable it but honestly we haven't used paropt much lately.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 74.75%. Comparing base (9d417c7) to head (9aa403f). :warning: Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pyoptsparse/testing/pyOpt_testing.py	66.66%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #414       +/-   ##
===========================================
- Coverage   85.97%   74.75%   -11.22%     
===========================================
  Files          22       24        +2     
  Lines        3315     3399       +84     
===========================================
- Hits         2850     2541      -309     
- Misses        465      858      +393     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[A-CGray]

@ewu63 @marcomangano , I believe this is ready for you to look at now.