-
Notifications
You must be signed in to change notification settings - Fork 36
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
Automatic launcher detection #120
Conversation
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.
Few comments. Most important being the module placement. Lets chat about that whenever you have a chance.
smartsim/experiment.py
Outdated
@@ -65,7 +65,9 @@ def __init__(self, name, exp_path=None, launcher="local"): | |||
:param exp_path: path to location of ``Experiment`` directory if generated | |||
:type exp_path: str, optional | |||
:param launcher: type of launcher being used, options are "slurm", "pbs", | |||
"cobalt", "lsf", or "local". Defaults to "local" | |||
"cobalt", "lsf", or "local", if set to "auto", |
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.
Period instead of comma
smartsim/experiment.py
Outdated
@@ -38,7 +38,7 @@ | |||
from .generation import Generator | |||
from .settings import settings | |||
from .utils import get_logger | |||
from .utils.helpers import colorize, init_default | |||
from .utils.helpers import colorize, detect_launcher, init_default |
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.
Is a user ever going to want to use this function? If so, we may considering creating a new module thats more user facing as the helpers are going to be moved to the _core
directory.
I think it's possible a user may want to inspect the value returned by detect_launcher
for specific use cases. I could go either way on this one. If you don't think so, just leave it.
If you do think we should have a module, names?
expUtils
?
wlmUtils
?
wlm
?
I could see the last one being good if we end up moving the slurm (allocation) api to the wlm
module.
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.
Yeah, definitely wlm
is the one which makes sense to me. I think you're right, users might want to include detect_launcher
explicitly in their code.
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.
Please see one comment and make sure that test case will pass. otherwise LGTM.
|
||
|
||
def test_launcher_detection(wlmutils): | ||
exp = Experiment("test-launcher-detection", launcher="auto") |
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.
What about the case where we run the local test suite on a system with Slurm? Does this still pass?
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.
@Spartee I changed my mind on this a couple of times. My current opinion is that we keep this test, but we add an exception in case wlmutils.get_test_launcher()=="local"
and there is another launcher on the system. This way, at least, the function is always tested, but the result is ignored on clusters/supercomputers, if there is a launcher and you have set SMARTSIM_TEST_LAUNCHER=local
.
This PR adds the automatic launcher detection to
Experiment
and tocreate_run_settings
andcreate_batch_settings
.Two detect the launcher, we check that all commands specified in the corresponding
<launcher>Commands.py
file are available.To distinguish between Cobalt and PBS, we call
qsub --version
and parse its output.