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

Hotfix: profiling initialized from the wrong thread in comm engine #622

Closed

Conversation

therault
Copy link
Contributor

After we introduced lazy initialization of the comm engine, we left the profiling initialization for the remote_dep comm engine be called from the main parsec thread. That initialization must be called from the communication thread, this patch moves it to the actual comm thread. (bug found and fixed by @devreal).

After we introduced lazy initialization of the comm engine, we left
the profiling initialization for the remote_dep comm engine be
called from the main parsec thread. That initialization must be called
from the communication thread, this patch moves it to the actual
comm thread. (bug found and fixed by @devreal).
@therault therault requested a review from a team as a code owner January 26, 2024 17:04
Copy link
Contributor

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

What is the real change here ? This patch moves a bunch of code around for no obvious reason.

@devreal
Copy link
Contributor

devreal commented Jan 27, 2024

Thanks @therault!

@bosilca The real change is that remote_dep_mpi_profiling_init should be called from within remote_dep_dequeue_main (the comm thread's main function) instead of remote_dep_ce_init (called by the main thread to spin up the communication backend). The rest is moving code around for correct ordering.

@bosilca
Copy link
Contributor

bosilca commented Jan 27, 2024

Can we just pre-decleare the function instead of moving all the code around ?

@abouteiller
Copy link
Contributor

We will use #626 instead

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.

4 participants