Validate Slurm Timing format #471
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #471 +/- ##
===========================================
- Coverage 90.81% 90.72% -0.10%
===========================================
Files 60 60
Lines 3834 3837 +3
===========================================
- Hits 3482 3481 -1
- Misses 352 356 +4 |
smartsim/wlm/slurm.py
Outdated
| delta = datetime.timedelta(hours=hours, minutes=minutes, seconds=seconds) | ||
| fmt_str = str(delta) | ||
| if delta.seconds // 3600 < 10: | ||
| fmt_str = "0" + fmt_str | ||
| return fmt_str |
There was a problem hiding this comment.
Thoughts on breaking these lines out into a helper function that can be used both here and in smartsim.settings.slurmSettings.SrunSettings._fmt_walltime? That way if we have to update how to format the walltime for slurm we only need to touch one spot.
There was a problem hiding this comment.
hm hm _fmt_walltime in smartsim.settings.slurmSettings.SrunSettings is never used, so Ill add it to set_walltime and break it out into a helper func, I was hesitant on that at first bc I wasnt sure about using helper funcs in outside folders but that makes more sense
| @@ -256,13 +256,9 @@ def _fmt_walltime(hours: int, minutes: int, seconds: int) -> str: | |||
| :param seconds: number of seconds to run job | |||
| :type seconds: int | |||
| :returns: Formatted walltime | |||
| :rtype | |||
| :rtype: str | |||
There was a problem hiding this comment.
good catch!! Thanks for fix!!
This PR merges in functionality to validate the timing format when requesting a slurm allocation. Previously, no check was required leading to the WLM responsibility to throw an error. With the new code, SmartSim will catch and throw.