Conversation
|
Setup declarations have been put in the main task files. I went with method 3 as that seemed to work best. Method 4 seemed to register the inherited classes under each instance that TaskSetup was imported under, so it didn't work to centrally track them. Method 5 worked but there wasn't a great way to robustly handle the naming of the classes without initializing them. I moved the external imports (eva, r2d2, jedi_bundle) into the execute methods of the tasks that need them, but there may be a better way of handling this |
shiklomanov-an
left a comment
There was a problem hiding this comment.
Nicely done! I really like how this is looking. I think having the task parameters in the same files as the task definitions will be very useful.
I flagged only a few (hopefully minor) things.
- Some of these are type annotation issues flagged by my type checker.
- There's also what looks like a bug related to
overrideas a file path vs. a dictionary. - Finally, there are a few documentation requests for implementation details and design decisions, to help future developers (since we're using slightly esoteric Python machinery to get this working).
(Re: Local import documentation --- if we're just using local imports for consistency across modules, that's totally acceptable and you can disregard my comments about those...but maybe add a line to the documentation somewhere about why we're using that general pattern, with reference to specific cases like eva and r2d2).
@Dooruk @rtodling -- Take a look the new task definition structure (any of the files under tasks) and let me know what you think. Don't worry too much about the underlying machinery; I'm more interested on what you think of this overall structure (compared to having all the task questions sit in a single giant YAML file / Python script like we had before; #314).
|
Wow, this is some impressive amount of change. Really appreciate the documentation, it would be quite confusing without it. When I first started using SWELL, I was able to include config variables inside the task scripts without the Overall, I'm happy with these changes. In the previous workflow templating iteration Abandoning single task_questions and suite_questions is a welcome change. Users would also see Since we have a limited number of seasoned users, I would be curious to hear from Yonggang and Maryam too, once this is ready for that stage. Some minor questions/details/nitpicks:
|
|
@Dooruk Thanks! To answer your questions:
class GcmRunJ(TaskSetup):
def set_attributes:
self.base_name = "RunGeos"
self.script = "{{experiment_path}}/GEOSgcm/forecast/gcm_run.j"
self.slurm = {}
self.additional_sections.append(
self.create_new_section('job', {'shell': 'csh'})
)
class Workflow:
...
def set_tasks(self) -> list:
self.tasks.append(GcmRunJ)For development purposes, there's also no reason you couldn't just add the section in plain text to the scheduling swell/src/swell/suites/3dvar/workflow.py Lines 101 to 107 in dabe9b2 ^ right here, for instance.
In all workflows I maintained that the scheduling section is templated using jinja, like so: swell/src/swell/suites/3dvar/workflow.py Lines 111 to 122 in dabe9b2 So that section of file construction is still handled in the same jinja logic. But technically, you can set the
|
This is the most recent version of the concept of updating the workflow templating to use python. This approach should be more straightforward and easier to understand than the previous version, as it uses the same jinja templating in the graph section to make it easier to read (though it doesn't necessarily have to). This pr also merges the task_runtimes and task_questions objects used by the previous pr into one object for simplicity. I'm aiming for this approach to allow for greater independence of the tasks from the suite creation system, to allow generating configs for single tasks and having mock values for code tests for each task, but that's not ready to show off yet.
I still need to merge develop into this and update the docs, but I wanted to open this up as a draft