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.

Sign up for GitHub

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

Jump to bottom

[WIP] MPI update #328

Merged
merged 47 commits into from
Dec 20, 2023
Merged

[WIP] MPI update #328

merged 47 commits into from
Dec 20, 2023
+526 −215

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Dec 8, 2023

This pull request makes the MPIEvaluator feature complete. It adds

  • centralized logging analogous to the MultiprocessingEvaluator and the IpyparallelEvaluator
  • handling of WorkingDirectoryModels
  • fixing the use of initializer functions for worker processes
  • make it possible to use chunksize as a kwarg on perform_experiment.
  • Various performance enhancements

Some more thoughts

  • The code has been tested on DelftBlue already using the new example_mpi_lake_model and associated slurm file.
  • DelftBlue sometimes seems to fail to spawn all workers. This results in a deadlock for the logging because creating an MPI intercommunicator is a blocking operation. The MPI intercommunicator that is created is only used to centralize logging. There are two possible fixes: have a timeout and fail the process or make centralized logging a user-specifiable choice.
  • The handling of WorkingDirectoryModels is implemented using the same structure as the MultiprocessingEvaluator but still needs to be tested

quaquel added 30 commits November 20, 2023 20:38
@quaquel
change to log message and log level in feature scoring
1739302 
fixes #261

columns with the same value for all entries don't matter for feature scoring, so they can be ignored.
@quaquel
adds a simple example using the MPI evaluator
d22414a
@quaquel
first step for making MPI work with WorkingDirectoryModels
be58e00 
bunch of things happening at the same time
* code reorganization, moving mpi-specific code into a separate module
* Return of initializer
* update to logging (hacky) to include rank information in all log messages
@quaquel
Update example_mpi_lake_model.py
d088b48
@quaquel
ongoing work
506999b
@quaquel
reorganizing evalutor code and renaming of modules
85ac783 
in preparation for future updates to the mpi code, all evaluator code has been reorganized. All evaluators go into their respective module, and code sthared among 2 or more goes into a new util package.

All relevant modules also have been renamed to future_{style of parallelization}
@quaquel
fixes location of mpi tests
2681924
@quaquel
some mocking fixes
f69cf80
@quaquel
Update futures_mpi.py
ff6d6d6
@quaquel
Update futures_multiprocessing.py
4fede84
@quaquel
attemp for doc fix
510c4fd
@quaquel
and one more
7e19066
@quaquel
another attempt
222d4f5
@quaquel
Update example_mpi_lake_model.py
278b684
@quaquel
Merge branch 'reorganization' into mpi_update
284ecd6
@quaquel
cleanup and merging reorganization into mpi_update
ca0d66a
@quaquel
logging works
0d144d9
@quaquel
unit testing for mpi logging
d87f3f0
@quaquel
Update ci.yml
5fd2214
@quaquel
Update futures_util.py
4953b03
@quaquel
unit test for intializer
59d8c6c
@quaquel
more unit tests
e7b1b30
@quaquel
more unit tests
39b004a
@quaquel
backport from mpi_update
04d02d8
@quaquel
Merge branch 'reorganization' into mpi_update
9f26e71
@quaquel
Update test_futures_mpi.py
88c1318
@quaquel
Merge branch 'master' into mpi_update
d365ab0
@quaquel
Update __init__.py
7ec29fc 
attempted import error fix
@quaquel
start of testing in delfblue
d5b6158
@quaquel
another attempted fix
042667d
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@quaquel quaquel added this to the 2.5.0 milestone Dec 8, 2023
@coveralls
Copy link

coveralls commented Dec 8, 2023 •
edited
Loading

Coverage Status

Changes unknown
when pulling 7482825 on mpi_update
into ** on master**.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 8, 2023

Looks like an impressive effort. If you want me to review at some point, let me know!

@quaquel
Copy link
Owner Author

quaquel commented Dec 8, 2023

No rush. This is just in preparation for our meeting next week. I hope to have tested WorkingDirectory models by then as well.

Most of the commits were just stupid tests on DelftBlue. I learned a lot about MPICH and OpenMPI as well as about how slurm interacts with MPI.

@EwoutH
Copy link
Collaborator

EwoutH commented Dec 8, 2023

Most of the commits were just stupid tests on DelftBlue. I learned a lot about MPICH and OpenMPI as well as about how slurm interacts with MPI.

That really describes my experience in Q1! There are so many hidden assumptions everywhere. So much trial and error.

Even for me, and I really like trial and error.

@EwoutH EwoutH marked this pull request as draft December 13, 2023 20:56
@quaquel quaquel marked this pull request as ready for review December 20, 2023 12:29
EwoutH
EwoutH approved these changes Dec 20, 2023
View reviewed changes
Copy link
Collaborator

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

TODO: Testen working directories

@quaquel quaquel merged commit a968cc8 into master Dec 20, 2023
@quaquel quaquel deleted the mpi_update branch December 20, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwoutH EwoutH EwoutH approved these changes

Assignees
No one assigned
Labels
enhancement performance
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@quaquel @coveralls @EwoutH