Raising error for reserved keywords under function parameter options in get_allocation

[juliaputko]

Added error if a user calls get_allocation with reserved keywords (nodes, account, time) as options eg. get_allocation(options={"account"="ACCOUNT"}).

Added test to ensure error is correctly thrown.

[MattToast]

LGTM pending a couple of knit picks and stripping the whitespace!!

[codecov]

Codecov Report

Merging #325 (295ae72) into develop (4c741be) will decrease coverage by 0.13%.
Report is 2 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #325      +/-   ##
===========================================
- Coverage    87.30%   87.18%   -0.13%     
===========================================
  Files           59       59              
  Lines         3522     3519       -3     
===========================================
- Hits          3075     3068       -7     
- Misses         447      451       +4     

Files Changed	Coverage Δ
smartsim/error/__init__.py	100.00% <ø> (ø)
smartsim/error/errors.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[ashao]

Very small change requested. Otherwise thanks for fixing this silent, but very weird piece of behavior!

[ashao]

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

[juliaputko]

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

[MattToast]

@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"""

[ashao]

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?

[ankona]

I see Matt's req for a ticket to handle the TODO, but otherwise looks great to me.

[ashao]

Looks great! Thank you for the change