Skip to content

Conversation

@A-CGray
Copy link
Member

@A-CGray A-CGray commented Oct 17, 2025

Purpose

Adds a _finalizeObjectives method that reduces any added objectives across all procs so that all procs have the same objectives. This is already done for design variables and constraints.

Expected time until merged

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • 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

I added a small unit test that checks that all procs have the same objectives, constraints and design variables after finalize is called.

Checklist

  • I have run ruff check and ruff format 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 A-CGray requested a review from a team as a code owner October 17, 2025 23:51
@A-CGray A-CGray requested review from ArshSaja and sabakhshi October 17, 2025 23:51
@codecov
Copy link

codecov bot commented Oct 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.72%. Comparing base (bee4701) to head (76e332f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
+ Coverage   86.27%   86.72%   +0.44%     
==========================================
  Files          24       24              
  Lines        3432     3435       +3     
==========================================
+ Hits         2961     2979      +18     
+ Misses        471      456      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@A-CGray A-CGray requested review from ewu63 and kanekosh and removed request for ArshSaja and sabakhshi October 18, 2025 00:57
Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

So I don't believe this is a bug, we've never supported distributed objectives before since we've never needed to, only constraints/DVs. In any case, I think this was a pretty non-documented feature and this change LGTM.

"""
if comm is None:
raise unittest.SkipTest("mpi4py not available, skipping test.")
if comm.size != self.N_PROCS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When does this condition happen?

Copy link
Member Author

@A-CGray A-CGray Oct 19, 2025

Choose a reason for hiding this comment

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

If you have mpi4py installed but you run the test through something other than testflo (e.g unittest or pytest)

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this sort of thing do not need to be handled, we explicitly say in the docs to run tests with testflo so I don't think we need to support running raw unittest.

Copy link
Member Author

@A-CGray A-CGray Oct 20, 2025

Choose a reason for hiding this comment

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

True, but whenever I want to interactively debug a failing test I have to run them with unittest and I'd rather not have to comment this test out every time I want to do that. I guess I could also try changing how the test works so it won't fail if run in serial, but it seemed easier to just do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll approve, but I don't think we should do things like this in general, because we have tons of MPI-based tests (here or in other repos) and we should not have to add these checks for each one. Maybe in the longer term we should move away from testflo-based testing to something more standard like pytest, it may not be sufficient for packages like ADflow but the parallel portions of pyOptSparse are minimal and easy to test.

@A-CGray
Copy link
Member Author

A-CGray commented Oct 19, 2025

So I don't believe this is a bug, we've never supported distributed objectives before since we've never needed to, only constraints/DVs. In any case, I think this was a pretty non-documented feature and this change LGTM.

Right, I don't think what I've implemented here actually supports objectives that are distributed at runtime (as in it still expects the same objective value to be returned on all procs when it calls masterfunc). Does pyOptSparse support constraints that are distributed at runtime? I didn't see anything in the code that suggests it does, but if it does then presumably I should add something similar to this PR?

@ewu63
Copy link
Collaborator

ewu63 commented Oct 20, 2025

Does pyOptSparse support constraints that are distributed at runtime? I didn't see anything in the code that suggests it does, but if it does then presumably I should add something similar to this PR?

No it does not, that functionality lives in multipoint. I've long thought that multipoint could be moved into pyOptSparse or at least seen as a "plugin" for this repo, but that's probably a long-term discussion. My original point was just that we had never had the need to run an allreduce on the objective, since typically we just have one objective function which is available on all procs.

@A-CGray A-CGray changed the title Fix bug that occurs when an objective is not added on all procs Support calling addObjective on subset of procs Oct 20, 2025
@ewu63
Copy link
Collaborator

ewu63 commented Oct 22, 2025

I would be OK with a patch release after this PR is merged, feel free to bump the version. Alternatively we can wait for #459

@A-CGray
Copy link
Member Author

A-CGray commented Oct 23, 2025

I would be OK with a patch release after this PR is merged, feel free to bump the version. Alternatively we can wait for #459

Let's do it in #459

@kanekosh kanekosh merged commit 19ad2c8 into main Oct 23, 2025
16 checks passed
@kanekosh kanekosh deleted the finalizeObjectives branch October 23, 2025 14:56
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.

3 participants