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

Replace SingleNodeLauncher with SimpleLauncher for MPI Mode #3418

Merged
merged 3 commits into from
May 16, 2024

Conversation

WardLT
Copy link
Contributor

@WardLT WardLT commented May 7, 2024

Description

Corrects a problem in the MPI mode implementation of HTEx. SingleNodeLauncher creates too many workers when running in MPI mode because it launches multiple copies of the executor onto the head node.

We want SimpleLauncher, which (instead) launches a single copy of the executor.

Changed Behaviour

Anything that uses the MPI mode will now create the desired number of workers.

Fixes

No issue, but see discussion in Slack.

Type of change

Choose which options apply, and delete the ones which do not apply.

  • Bug fix
  • Update to human readable text: Documentation/error messages/comments

SingleNodeLauncher creates too many workers when running in MPI mode
@benclifford benclifford requested a review from yadudoc May 7, 2024 20:58
@WardLT WardLT mentioned this pull request May 8, 2024
4 tasks
@@ -1,4 +1,3 @@
import logging
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have been trying to keep tidyups in separate PRs from functionality changes, with a goal of the changes being on-topic - this stuff is super confusing to debug when it turns out eg. something broke because of a changed import rather than a launcher change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made this into PR #3436

@benclifford benclifford merged commit 3e4f101 into Parsl:master May 16, 2024
6 checks passed
@benclifford
Copy link
Collaborator

i changed "executor" to "worker pool" in the merge message - another note for #3426

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

Successfully merging this pull request may close these issues.

2 participants