-
Notifications
You must be signed in to change notification settings - Fork 37
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
Integrate PalsMpiexecSettings into Experiment factory methods #343
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #343 +/- ##
===========================================
+ Coverage 87.31% 88.23% +0.91%
===========================================
Files 59 59
Lines 3531 3552 +21
===========================================
+ Hits 3083 3134 +51
+ Misses 448 418 -30
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I gotta ask if we ran the tests on any PALS machine because I'm the baaad guy
@@ -64,6 +64,7 @@ | |||
by_launcher: t.Dict[str, t.List[str]] = { | |||
"slurm": ["srun", "mpirun", "mpiexec"], | |||
"pbs": ["aprun", "mpirun", "mpiexec"], | |||
"pals": ["mpiexec"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also support aprun
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is not yet, as I remember there were some issues. But we will have to in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm inclined to agree as I didn't specifically test against aprun
. I'll be sure to throw in a ticket to track the future work needed!
@al-rigazzi I manually scaled down a couple of the LMK if you think we should hold this off until we can do a more substantial test run!! |
Registers the
PalsMpiexecSettings
class in SmartsSim'sExperiment
factory methods so that the correct_BaseMpiSettings
class is returned depending on the launcher the user is intending to use: