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

examples: Update MPI tutorial notebook and scripts #1923

Merged
merged 4 commits into from Jun 7, 2022

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented May 20, 2022

Update the setup script and notebook.
Add deterministic output (--no-stream)

Much needed after 3 years.

@georgebisbas georgebisbas added the MPI mpi-related label May 20, 2022
@georgebisbas georgebisbas self-assigned this May 20, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #1923 (af10183) into master (45c75a7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1923   +/-   ##
=======================================
  Coverage   89.60%   89.60%           
=======================================
  Files         211      211           
  Lines       35941    35941           
  Branches     5414     5414           
=======================================
  Hits        32205    32205           
  Misses       3232     3232           
  Partials      504      504           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c75a7...af10183. Read the comment docs.

@@ -6,11 +6,6 @@ IPYTHONDIR=~/.ipython
# Create a new profile, called "mpi"
ipython profile create --parallel --profile=mpi

# Add the following line as per instructions from
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why are you dropping this? unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the machinery in line 12 has been deprecated.
The option engines=mpi I am using here:

https://github.com/devitocodes/devito/pull/1923/files#diff-4acc409654fcff44dde5c96e156d25a86f29d10a612d110f245af6f76a47a443R25

is taking care of instructing the MPI launchers.

@FabioLuporini
Copy link
Contributor

Thank you. So, you've tested it locally, right? and it still does work? nice

@FabioLuporini FabioLuporini added examples examples and removed MPI mpi-related labels May 24, 2022
Copy link
Contributor Author

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

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

Maybe this can be merged if happy

@@ -6,11 +6,6 @@ IPYTHONDIR=~/.ipython
# Create a new profile, called "mpi"
ipython profile create --parallel --profile=mpi

# Add the following line as per instructions from
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the machinery in line 12 has been deprecated.
The option engines=mpi I am using here:

https://github.com/devitocodes/devito/pull/1923/files#diff-4acc409654fcff44dde5c96e156d25a86f29d10a612d110f245af6f76a47a443R25

is taking care of instructing the MPI launchers.

@georgebisbas
Copy link
Contributor Author

Thank you. So, you've tested it locally, right? and it still does work? nice

Tested for me and alongside Kene as well

@mloubout
Copy link
Contributor

Can it be re-enable in the ci or is new version still notnworking

ver=$(mpiexec --version)
if [[ $ver == *"open-mpi"* ]]; then
# OpenMPI need to be told that is allowed to oversubscribe cores
rm $IPYTHONDIR/profile_mpi/ipcluster_config.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete any previous profiles in host.

@georgebisbas georgebisbas linked an issue May 25, 2022 that may be closed by this pull request
# ipcluster start --profile=mpi -n 4 --daemon
# py.test --nbval examples/mpi
# ipcluster stop --profile=mpi
pip3 install "ipyparallel<8.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

@georgebisbas out of curiosity, why <8.4 ? and shouldn't this rather be added to the "Install dependencies" section above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thought of it, maybe added in requirements-optional.txt ? (8.3 is current version)

@FabioLuporini
Copy link
Contributor

@georgebisbas we also probably need to add some NBVAL_IGNORE_OUTPUT or NBVAL_SKIP?

@georgebisbas georgebisbas force-pushed the mpi_notebook_refresh branch 4 times, most recently from 23a2885 to 2321176 Compare May 30, 2022 09:18
Copy link
Contributor

@mloubout mloubout left a comment

Choose a reason for hiding this comment

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

Almost there, two small comments

@@ -43,14 +43,16 @@ jobs:
- name: Install dependencies
run: |
pip install --upgrade pip
pip install -e .
pip install -e .[extras]
Copy link
Contributor

Choose a reason for hiding this comment

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

[extras,mpi]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1,3 +1,3 @@
matplotlib
ipyparallel
ipyparallel<8.4
Copy link
Contributor

Choose a reason for hiding this comment

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

move to mpi requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@georgebisbas georgebisbas changed the title mpi: Update script and notebooks examples: Update MPI tutorial notebook and scripts Jun 6, 2022
@@ -1,3 +1,3 @@
matplotlib
ipyparallel
ipyparallel<8.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -43,14 +43,16 @@ jobs:
- name: Install dependencies
run: |
pip install --upgrade pip
pip install -e .
pip install -e .[extras]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1 +1,2 @@
mpi4py
mpi4py<4.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NFR: this was added because soon mpi4py will go to version 4 after long time of minor upgrades

Copy link
Contributor

Choose a reason for hiding this comment

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

Does dependabot actually understand this is a dependency file as well?

@FabioLuporini FabioLuporini merged commit 5e0f6d0 into master Jun 7, 2022
@FabioLuporini FabioLuporini deleted the mpi_notebook_refresh branch June 7, 2022 10:03
@FabioLuporini
Copy link
Contributor

Merged, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples examples
Projects
Development

Successfully merging this pull request may close these issues.

mpi notebook and ipyparallel race conditions
3 participants