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

added duplicate comm #940

Merged
merged 10 commits into from
Mar 30, 2022
Merged

added duplicate comm #940

merged 10 commits into from
Mar 30, 2022

Conversation

Dhruv454000
Copy link
Contributor

Description

Duplicate MPI_COMM_WORLD for the default communicator.

Changes proposed:

Added a duplicate of MPI_COMM_WORLD to make library more independent.

@mtar
Copy link
Collaborator

mtar commented Mar 25, 2022

GPU cluster tests are currently disabled on this Pull Request.

@Dhruv454000
Copy link
Contributor Author

@mtar any changes required?

@mtar
Copy link
Collaborator

mtar commented Mar 25, 2022

Thank you. There is one minor issue: changelog.md needs to be updated under the title "Pending Additions". It might be useful to have a short comment in the code, too.

@mtar mtar linked an issue Mar 25, 2022 that may be closed by this pull request
@Dhruv454000
Copy link
Contributor Author

@mtar done with changes.

@Dhruv454000
Copy link
Contributor Author

@mtar can you pls review.

@ghost
Copy link

ghost commented Mar 28, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #940 (d71d511) into main (2b10dd2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #940   +/-   ##
=======================================
  Coverage   95.47%   95.47%           
=======================================
  Files          64       64           
  Lines        9875     9877    +2     
=======================================
+ Hits         9428     9430    +2     
  Misses        447      447           
Flag Coverage Δ
gpu 94.56% <100.00%> (+<0.01%) ⬆️
unit 91.09% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
heat/core/communication.py 96.77% <100.00%> (+0.01%) ⬆️

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 2b10dd2...d71d511. Read the comment docs.

@Dhruv454000
Copy link
Contributor Author

@ClaudiaComito any changes are required?

@ClaudiaComito
Copy link
Contributor

run tests

@ClaudiaComito
Copy link
Contributor

@Dhruv454000 thanks a lot, I just started the tests on GPU, I think all checks will pass afterwards

@ClaudiaComito
Copy link
Contributor

run tests

@ClaudiaComito
Copy link
Contributor

run tests

1 similar comment
@ClaudiaComito
Copy link
Contributor

run tests

@ClaudiaComito
Copy link
Contributor

run tests

@Dhruv454000
Copy link
Contributor Author

@ClaudiaComito is there any issue?

@ClaudiaComito
Copy link
Contributor

run tests

@ClaudiaComito
Copy link
Contributor

@ClaudiaComito is there any issue?

I think the cutoff time was exceeded on the GPU tests, because of the new tests introduced by the latest PR merge. Now the cutoff has been extended and everything works. I'm going to approve and merge this.

Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Dhruv454000 for taking this on and congratulations on your first contribution here! 👏🏼

@ClaudiaComito ClaudiaComito merged commit 5f5c7b6 into helmholtz-analytics:main Mar 30, 2022
@Dhruv454000 Dhruv454000 deleted the dhruv branch March 30, 2022 14:45
@ClaudiaComito ClaudiaComito mentioned this pull request Apr 21, 2022
4 tasks
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.

Duplicate MPI Communicator
3 participants