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

Add documentation about deadlock in mpi processes #2758

Merged
merged 4 commits into from
May 3, 2023

Conversation

jessica-mitchell
Copy link
Contributor

This PR add a note in the documentation in the distributed computing section of the parallel computing page, regarding the issue resolved in #2749.

See output here

@jessica-mitchell jessica-mitchell added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels May 2, 2023
@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation May 2, 2023
@jessica-mitchell jessica-mitchell changed the title Add documentation about deadlock in parallel computing Add documentation about deadlock in mpi processes May 2, 2023
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

@jessica-mitchell Thanks! I have some minor suggestions for the example code, but overall this looks good and is a very useful addition to the documentation.

doc/htmldoc/hpc/parallel_computing.rst Outdated Show resolved Hide resolved
doc/htmldoc/hpc/parallel_computing.rst Outdated Show resolved Hide resolved
Documentation automation moved this from In progress to Review May 2, 2023
Co-authored-by: Nicolai Haug <39106781+nicolossus@users.noreply.github.com>
Copy link
Contributor

@JoseJVS JoseJVS left a comment

Choose a reason for hiding this comment

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

Great! This is exactly what was needed. Perhaps could you add a sentence like:

The PyNEST API was designed so that all parallel operations are handled through the nestkernel, hence the users don't need to handle MPI communication from the python script.
Ideally the script should be written in a serial manner, however if specific operations unrelated to the simulation execution are needed, the NEST API offers the Rank() function to retrieve the MPI rank of the process and handle process specific tasks.
Note that one should never call any ....

So that it is clear that parallelization is fully handled from the kernel and not the python script.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@jessica-mitchell Thanks for this PR! I wonder if it would not work even better if the "Important" box came at the end of the "MPI related commands" section that is now just below the "Important" box. Also, in the "Run distributed simulations" section, an "r" is missing in "distibuted simulation scripts". Could you fix that as part of this PR?

Concerning the suggestion by @JoseJVS, I think this is pretty much covered if one looks at the "Using distributed computing" section as a whole, so adding another sentence would make it a bit repetitive.

@heplesser heplesser merged commit 7d9150c into nest:master May 3, 2023
20 checks passed
Documentation automation moved this from Review to Done May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Documentation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants