Add new RunSettings Methods #170
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #170 +/- ##
==========================================
Coverage ? 81.48%
==========================================
Files ? 57
Lines ? 2954
Branches ? 0
==========================================
Hits ? 2407
Misses ? 547
Partials ? 0 |
|
@al-rigazzi Thanks for the review! Issue #161 goes into more detail, but basically the deciding factor for whether or not an option got made into method on the base class was whether or not is the the same or very similar functionality existed on at least two of the supported interfaces ( I did my best to try and parse docs for each of the interfaces and I think I got most of the common options (at least all the options I understood). If you can think of any other arguments I missed, do lmk and I can add them in asap!! |
There was a problem hiding this comment.
Good stuff! A couple things
-
there are still gaps between the implementations. ex.
SrunSettingshasset_nodesand few others do each class should have the exact same methods available that will raise warnings in the case where it's not implemented. there should not be anAttributeErrorraised if I call the same methods across all classes. this is with the exception ofJsrunSettingswhich needs weird special methods for resource sets. more on this later -
I think we should provide a better interface for time across all the settings. I think we should be doing the translation to a string walltime. All the batch settings implement
set_walltimeand I think we should also provideset_timewhich takes in the suggestion i commented above as arguments and then formats and callsset_walltimeif its aBatchSettingsclass. -
@al-rigazzi why does
AprunSettingsrandomly implementset_walltime? was there a need for that? you're on the git blame. -
Instead of
set_timeoutlets call itset_timeand implement the suggestion above. One helper method in the base class to do the time conversion and those classes that need to can override. -
set_hostlist_from_fileis another I think that could be implemented across all of them. -
@al-rigazzi Is there a strict reason why we need to continue supporting the resource set semantics of LSF? can we get away with just node/task/etc if we mention that this is an alias for resource sets? I would like to get consistent.
| self.set_cpus_per_rs(cpus_per_task) | ||
| self.set_cpus_per_rs(int(cpus_per_task)) | ||
|
|
||
| def set_memory_per_rs(self, memory_per_rs): |
There was a problem hiding this comment.
@al-rigazzi do you think we should continue to have these aliases? or just implement the specification?
There was a problem hiding this comment.
@Spartee (cc @MattToast): Answering this and your general comment: I am not really sure we can map everything in LSF to node/task/etc. We can try, and simply say that for everything else, a standard run_arg method can be used.
About set_walltime in AprunSettings. I cannot think of a reason why we should have it now, I guess it made sense in an old version of the library, possibly.
|
also, whitespace!!!! purge it! ha |
Append a change to #170. Changes call to `SrunSettings.format_env_vars` to `SrunSettings.format_comma_sep_env_vars` in `SrunStep`
Adds new
RunSettingsmethods to grow the list of interface functions that are available across allRunSettingsobjects where possible. Methods added for similar arguments that are shared by two or more of the existingRunSettingssub-classes.Do let me know if I am missing any additional methods for arguments that should be included, or implementations for existing
RunSettings.