Fix #23: Divide the launch modes into default vs internal#25
Fix #23: Divide the launch modes into default vs internal#25Intuity merged 9 commits intoIntuity:mainfrom
Conversation
Default is to assume the user is calling Gator and therefore to treat a single Job as though it is to be launched as per a normal JobGroup/JobArray, i.e. via the scheduler. Internal mode tells Gator it's executing as a child on the actual runner instance, and therefore to expect a single job to run locally regardless of scheduler options.
|
Tested on my cluster deployment of Gator on Slurm and it works. |
I hadn't appreciated that Gator invokes itself for nested JobArray and JobGroup specs. This seems like an odd choice as it might result in the JobArray/JobGroup being dispatched onto individual Slurm nodes and then further distribution is restricted to just that slurm node. May need further investigation/consideration.
… under internal mode.
Intuity
left a comment
Intuity
left a comment
There was a problem hiding this comment.
The tests and handling look good, I'm just not 100% convinced on the necessity of the introduction of cast and a second minor style point around the assertion
| # - Passed in directly (when used as a library | ||
| elif spec is not None and isinstance(spec, (Job, JobArray, JobGroup)): | ||
| pass | ||
| parsed_spec = cast(SpecBase, spec) |
There was a problem hiding this comment.
I don't understand the need for this - introduction of this parsed_spec variable seems unnecessary, and the cast only exists to assist type checking (which is then replicated by line 101-104)
There was a problem hiding this comment.
Introduction of this variable is necessary to stop mypy complaining like crazy about the types. The cast is necessary because of the inferred type of Spec not matching the specified type of parsed_spec.
There was a problem hiding this comment.
The check is repeated again later because the other if cases also cause MyPy to relax the inferred type, which causes the typecheck of parsed_spec to go awry later in the file.
There was a problem hiding this comment.
My suggestion is mypy is basically worthless if we're not going to actually fix the errors/warnings it generates. The current codebase has mypy configured and generates a very large number of issues. Happy to address them, or we could remove MyPy if it's just generating noise.
There was a problem hiding this comment.
I'm not sure where mypy is configured - I can't see any evidence of it existing in the codebase.
Having a type checker and fixing issues is probably a sensible thing to do - but it's worth considering using something like Astral ty for speed, and adding it into the precommit/CI flow.
There was a problem hiding this comment.
Huh, I wonder where my VSCode environment is running it from. I get a lot of stuff show up in my Problems view, and I assumed it'd come from the poetry install. Maybe one of the VSCode Python extensions has it enabled by default. Sorry about that. Either way, I think it's worth keeping the code type-correct (it's a personal dislike that so much Python code reuses variables mutating the type halfway through a function. I get that "it's Pythonic" but it just seems wrong to me.)
Co-authored-by: Peter Birch <peter.birch@intuity-design.co.uk>
2 participants
Default is to assume the user is calling Gator and therefore to treat a single Job as though it is to be launched as per a normal JobGroup/JobArray, i.e. via the scheduler.
Internal mode tells Gator it's executing as a child on the actual runner instance, and therefore to expect a single job to run locally regardless of scheduler options.