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

Improve OpenMP affinity warning to include MPI concerns #6185

Merged
merged 3 commits into from
Jun 9, 2023
Merged

Improve OpenMP affinity warning to include MPI concerns #6185

merged 3 commits into from
Jun 9, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 6, 2023

Tentative resolution for #6180

@dalg24 dalg24 mentioned this pull request Jun 6, 2023
Closed
@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 7, 2023 •
edited

IMO, we should not remove the warning.
I would vote to improve it for when MPi is detected . Similar to what @Chrismarsh suggested here #6180 (comment) but maybe making the message more elaborate.

if (mpi_detected){
  R"WARNING(Kokkos::OpenMP::initialize in hybrid MPI environment: WARNING: OMP_PROC_BIND environment variable not set
    Something that can inform users about what to do... for example to leave OMP_PROC_BIND empty or if they know what they are doing they can have finer grained control but require a more in-depth knoweledge of MPI and OPENMP interop
}
else{
 // same as it is now
}

@dalg24 dalg24 closed this Jun 8, 2023
@dalg24 dalg24 reopened this Jun 8, 2023
dalg24
dalg24 commented Jun 8, 2023
View reviewed changes
core/src/OpenMP/Kokkos_OpenMP_Instance.cpp
@@ -241,13 +241,19 @@ void OpenMPInternal::initialize(int thread_count) {
}

{
if (Kokkos::show_warnings() && nullptr == std::getenv("OMP_PROC_BIND")) {
if (Kokkos::show_warnings() && !std::getenv("OMP_PROC_BIND")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't mean to commit. Let me know if you want me to undo.

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 9, 2023 •
edited

looks good to me! (CI failure unrelated)

@crtrott crtrott changed the title Do not warn about OMP_PROC_BIND not being set if we can detect MPI Improve OpenMP affinity warning to include MPI concerns Jun 9, 2023
@crtrott crtrott merged commit 6ca60c3 into kokkos:develop Jun 9, 2023
29 of 30 checks passed
@crtrott
Copy link
Member

crtrott commented Jun 9, 2023

CI failure was unrelated

@dalg24 dalg24 mentioned this pull request Jun 9, 2023
Closed
@dalg24 dalg24 deleted the mpi_omp_warning branch June 9, 2023 17:14
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
@dalg24 @nliber
Improve OpenMP affinity warning to include MPI concerns (kokkos#6185)
10a8c8d 
* Detect MPI execution environment

* Do not warn about OMP_PROC_BIND not being set when MPI is used

* Improve warning message and is_mpi_exec -> mpi_detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crtrott crtrott crtrott approved these changes

@fnrizzi fnrizzi fnrizzi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dalg24 @fnrizzi @crtrott