Requirements By Job Type And BatchSystem.submit
#489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added `_job_resources_from_size_and_inference` to calculate job resources from job size and inference type along with corresponding docs and tests. This is a temporary solution since a method like this should be added to GH-402/GH-432 rather than hard coded in `gempyor.batch`. Simpler to do this way for MVP's sake rather than trying to abstract code in a way that won't be reusable in the future.
Added `gempyor.utils._format_cli_options` for formatting CLI options in calls to executables via `subprocess.run` and similar.
Added `gempyor.testing.sample_script` utility for creating an executable (or optionally non-executable) script for unit tests that need to call out to some command.
* Added an abstract method to `BatchSystem` for `submit` which submits a job. * Implemented said method for `SlurmBatchSystem` and `LocalBatchSystem`. * Added `JobSubmission` class to represent result of job submission with a job id. * Corresponding docs and unit tests.
TimothyWillard
requested review from
pearsonca,
emprzy,
fang19911030,
anjalika-nande,
saraloo,
MacdonaldJoshuaCaleb and
jcblemai
emprzy
approved these changes
Jan 31, 2025
pearsonca
reviewed
Jan 31, 2025
| ['-o=/path/to/output.log'] | ||
| >>> _format_cli_options({"opt1": "```", "opt2": "$( echo 'Hello!')"}) | ||
| ["--opt1='```'", '--opt2=\'$( echo \'"\'"\'Hello!\'"\'"\')\''] | ||
| """ |
There was a problem hiding this comment.
how does this handle {"opt1": ["foo", "bar", "fizz", "buzz"]}?
There was a problem hiding this comment.
Addressed in a60ddce.
| @@ -319,6 +487,77 @@ class LocalBatchSystem(BatchSystem): | |||
|
|
|||
| name = "local" | |||
|
|
|||
| def submit( | |||
There was a problem hiding this comment.
This feels and the next concrete implementation feels a bit copy-pasta. are there elements of these that should be pulled into the parent class, and then called in these with, say, super().log(...)
There was a problem hiding this comment.
Addressed in ea1c864.
| @@ -159,6 +164,123 @@ class JobSize(BaseModel): | |||
| blocks: PositiveInt | |||
|
|
|||
|
|
|||
| def _job_resources_from_size_and_inference( | |||
| job_size: JobSize, | |||
| inference: Literal["emcee"] | None, | |||
There was a problem hiding this comment.
I kinda think I'd prefer this as inference: Literal["emcee", "r"] = "emcee" and then error on an argument it doesn't understand
There was a problem hiding this comment.
I agree with Carl on this
There was a problem hiding this comment.
Addressed in f7c99e6.
Change the `inference` arg from `Literal["emcee"] | None` to `Literal["emcee", "r"]` to reflect that the absense of inference method refers to "r" inference.
Changed `_format_cli_options` to either take a string or an iterable of strings as values. Previously had accepted any value, but only accepting string(s) makes it the responsibility of the caller to format objects to a string(s). Added an option to configure iterable formatting as well.
TimothyWillard
added
gempyor
Consolidated `LocalBatchSystem.submit` and `SlurmBatchSystem.submit` into `_submit_via_subprocess` internal helper to deduplicate code.
Added extensive unit tests for `gempyor.batch._submit_via_subprocess` internal utility.
Open
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes.
This pull request:
_job_resources_from_size_and_inferenceto give a defaultJobResourcesfromJobSizeand inference method name*,BatchSystem.submitto submit jobs to a batch system which can return aJobSubmissionwhich is a thin extension ofsubprocess.CompletedProcesswith ajob_idattribute,submitforLocalBatchSystemandSlurmBatchSystem,_format_cli_optionstogempyor.utilsfor formatting CLI arguments,sample_script.*There were discussions of creating a
JobTypeABC and have EMCEE and legacy implement this and then add this method to that (similar toBatchSystem). I decided against this to 1) develop the MVP faster, and 2) I think this method would have to be added to both the simulator and inference ABCs (see GH-402 & GH-432 respectively), because the inference method would have to call the simulator one and use that result in it's own calculation.Does this pull request make any user interface changes? If so please describe.
The user interface changes are some API changes to
gempyor.batchwhich have been documented in docstrings, has not been documented in GitBook yet. I think documenting there is more appropriate once further along in the product.What does your pull request address? Tag relevant issues.
This pull request addresses GH-487, spun out of GH-394. After this PR will be actually creating the
flepimop submitCLI action using all of this infrastructure.