-
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
Changes from 5 commits
6f8eea2
46dd28d
8a6e8b4
67d2a81
e08f495
cfaeb37
5b22015
3af235e
b5c7e12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,8 +24,8 @@ | |
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
|
||
import time | ||
import os.path as osp | ||
import time | ||
from os import getcwd | ||
from pprint import pformat | ||
|
||
|
@@ -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 | ||
|
||
logger = get_logger(__name__) | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Period instead of comma |
||
an attempt will be made to find an available launcher on the system. | ||
Defaults to "local" | ||
:type launcher: str, optional | ||
""" | ||
self.name = name | ||
|
@@ -76,6 +78,10 @@ def __init__(self, name, exp_path=None, launcher="local"): | |
raise NotADirectoryError("Experiment path provided does not exist") | ||
exp_path = osp.abspath(exp_path) | ||
self.exp_path = init_default(osp.join(getcwd(), name), exp_path, str) | ||
|
||
if launcher == "auto": | ||
launcher = detect_launcher() | ||
|
||
self._control = Controller(launcher=launcher) | ||
self._launcher = launcher.lower() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import numpy as np | ||
import torch | ||
import torch.nn as nn | ||
|
||
from smartredis import Client | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,3 +123,8 @@ def test_summary(fileutils): | |
assert m.type == row["Entity-Type"] | ||
assert 0 == int(row["RunID"]) | ||
assert 0 == int(row["Returncode"]) | ||
|
||
|
||
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 commentThe 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 commentThe 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 |
||
assert exp._launcher == wlmutils.get_test_launcher() |
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 includedetect_launcher
explicitly in their code.