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.

Sign up for GitHub

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

Jump to bottom

Raising error for reserved keywords under function parameter options in get_allocation #325

Merged
merged 25 commits into from
Jul 31, 2023
Merged

Raising error for reserved keywords under function parameter options in get_allocation #325

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
419df86
tests in test_slurm_allocation and error check in slurm
Jul 20, 2023
e0b6320
corrected test to dict
Jul 20, 2023
53a8173
removed stuff after it fails out in test
Jul 20, 2023
53cba50
parameterized tests
Jul 21, 2023
f12c455
naming edit
Jul 21, 2023
081e8c8
Whitespace removal, removed unused params, and changed error raised t…
Jul 21, 2023
0ba0cbb
raising correct error in test
Jul 21, 2023
965a8b4
removed unused error
Jul 24, 2023
8f7a745
removed whitespace
Jul 24, 2023
3b89865
deleting test that allows bad behaviour
Jul 24, 2023
9863cd1
checking to make sure error is correctly thrown
Jul 25, 2023
c491dc1
removed unused
Jul 26, 2023
4a189e0
added validation before loop, and improved test
Jul 27, 2023
8c90c7c
Merge branch 'develop' of github.com:CrayLabs/SmartSim into jp116
Jul 27, 2023
1934e0a
typo on intersection
Jul 27, 2023
8926464
test
Jul 27, 2023
86ae6c9
typo
Jul 27, 2023
937e18e
fixing mypy error
Jul 27, 2023
e4f6870
black line length fixing
Jul 27, 2023
80d56af
lint error fix
Jul 27, 2023
64bff4f
lint error fix
Jul 27, 2023
218b1b1
removed TODO
Jul 28, 2023
695982d
added new/more accurate error
Jul 28, 2023
2adba67
cleaned lint
Jul 28, 2023
295ae72
changed parent for error
Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion smartsim/wlm/slurm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from .._core.launcher.slurm.slurmParser import parse_salloc, parse_salloc_error
from .._core.launcher.util.launcherUtil import ComputeNode, Partition
from .._core.utils.helpers import init_default
from ..error import AllocationError, LauncherError, SmartSimError
from ..error import AllocationError, LauncherError, SmartSimError, SSConfigError
from ..log import get_logger

logger = get_logger(__name__)
Expand Down Expand Up @@ -253,6 +253,8 @@ def _get_alloc_cmd(nodes: int,
salloc_args += [prefix + opt, str(val)]
else:
salloc_args += ["=".join((prefix + opt, str(val)))]
else:
raise SSConfigError("Expecting time, nodes, account as an argument")
juliaputko marked this conversation as resolved.
Show resolved Hide resolved

return salloc_args

Expand Down
8 changes: 7 additions & 1 deletion tests/full_wlm/test_slurm_allocation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

import pytest

from smartsim.error import AllocationError
from smartsim.error import AllocationError, SSConfigError
Copy link
Member

Choose a reason for hiding this comment

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

Were you intending to raise the ValueError on line 58 with a SSConfigError? That's probably a more accurate and specific error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have gotten mixed reviews on which error I should throw. I believe it was @MattToast that suggested I switch to a value error

Copy link
Member

Choose a reason for hiding this comment

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

@ashao, my argument was that in the past the SSConfigError has been used for when SmartSim has not been correctly built (e.g. the user has not run smart build). For example:

raise SSConfigError(
    "RedisAI dependency not found. Build with `smart` cli "
    "or specify RAI_PATH"
)

I'd be okay as creating a new, more appropriate error, but I'm going to strongly push against using SSConfigError as it is a subclass of an SSInternalError and we raise here due to bad user input, not due to something wrong within SmartSim:

class SSInternalError(Exception):
    """
    SSInternalError is raised when an internal error is encountered.
    """

class SSConfigError(SSInternalError):
    """Raised when there is an error in the configuration of SmartSim"""

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. I'd prefer a new error class just to give us a bit better granularity on the exceptions we can catch. ValueError seems a bit too generic for this one for my liking. @juliaputko , @MattToast how do you feel about SSReservedKeyword?

from smartsim.wlm import slurm

# retrieved from pytest fixtures
Expand All @@ -52,6 +52,12 @@ def test_get_release_allocation_w_options(wlmutils):
time.sleep(5) # give slurm a rest
slurm.release_allocation(alloc)

@pytest.mark.parametrize("func_parameters,test_parameters", [("time","TIME"),("nodes","NODES"),("account","ACCOUNT")])
def test_get_allocation_bad_params(func_parameters,test_parameters):
"""test get_allocation with reserved keywords as option """
juliaputko marked this conversation as resolved.
Show resolved Hide resolved
options = {"ntasks-per-node": 1}
juliaputko marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(SSConfigError):
alloc = slurm.get_allocation(options={func_parameters:test_parameters})
juliaputko marked this conversation as resolved.
Show resolved Hide resolved

# --------- Error handling ----------------------------

Expand Down
Loading