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

Making sure the MultiprocessingParallelManager cleans up when done #1211

Merged
merged 12 commits into from
Nov 21, 2023

Conversation

bknueven
Copy link
Contributor

@bknueven bknueven commented Nov 21, 2023

Fixes/Resolves: #1158

Summary/Motivation:

See #1158. This will call kill to clean up any hanging Processes at the end of the run using the MultiprocessingParallelManager.

Changes proposed in this PR:

  • See above.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  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.

@bknueven bknueven marked this pull request as ready for review November 21, 2023 15:26
@bknueven bknueven marked this pull request as draft November 21, 2023 18:40
@bknueven
Copy link
Contributor Author

I am going to adjust the semantics of this to make it identical in behavior to the prior implementation.

@lbianchi-lbl
Copy link
Contributor

@watertap-org/parameter-sweep just testing team notifications for the newly created https://github.com/orgs/watertap-org/teams/parameter-sweep org team.

@bknueven
Copy link
Contributor Author

The timeout tests are run here: https://github.com/bknueven/watertap/actions/runs/6948646192?pr=17.

c241426 takes a much different approach and doesn't change the core algorithm. But I think it does address the issue.

@bknueven bknueven changed the title Use multiprocessing.Pool in MultiprocessingParallelManager Making sure the MultiprocessingParallelManager cleans up when done Nov 21, 2023
@bknueven bknueven marked this pull request as ready for review November 21, 2023 21:41
Copy link
Contributor

@avdudchenko avdudchenko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (166e4a9) 91.21% compared to head (161944a) 94.91%.

Files Patch % Lines
...tools/parallel/multiprocessing_parallel_manager.py 64.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1211      +/-   ##
==========================================
+ Coverage   91.21%   94.91%   +3.70%     
==========================================
  Files         343      344       +1     
  Lines       34900    34919      +19     
==========================================
+ Hits        31834    33144    +1310     
+ Misses       3066     1775    -1291     

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

@lbianchi-lbl lbianchi-lbl merged commit 7c6ae2a into watertap-org:main Nov 21, 2023
@bknueven bknueven deleted the issue-1158-clean branch November 22, 2023 15:29
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Nov 30, 2023
lbianchi-lbl added a commit to watertap-org/parameter-sweep that referenced this pull request May 1, 2024
…atertap-org/watertap#1211)

* Try adding explicit timeout where possible/easy

* Try one more timeout

* Add quick n dirty way to shut down multiprocessing worker processes

* Try using EAFP instead of checking Queue.empty()

* Make timeout configurable with default value

* Refactor to use logging

* Reorder imports

* remove timeout from concurrent.futures.wait

* trying Pool implementation

* Revert "trying Pool implementation"

This reverts commit 5354b1794ae1195b1f9222f723e141908d7cfc76.

* try using kill

* pylint

---------

Co-authored-by: Ludovico Bianchi <lbianchi@lbl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parameter-sweep Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_multiprocessing_parallel_manager.py causes occasional CI failures after 6h timeout
5 participants