Add some methods and initialize task_config in task class #29
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #29 +/- ##
===========================================
- Coverage 47.93% 47.91% -0.03%
===========================================
Files 18 18
Lines 1650 1657 +7
Branches 335 336 +1
===========================================
+ Hits 791 794 +3
- Misses 799 803 +4
Partials 60 60 ☔ View full report in Codecov by Sentry. |
src/wxflow/task.py
Outdated
| def stage_files(self, path: str) -> None: | ||
| """ | ||
| Pass dictionary, created by parsing Jinja2-templated YAML, to file handler | ||
| """ | ||
|
|
||
| FileHandler(parse_j2yaml(path, self.task_config)).sync() |
There was a problem hiding this comment.
I don't think this belongs here in this form.
If we want a stage method in the task, its input should be the a fully formed list and not something that needs to be rendered.
There was a problem hiding this comment.
That's basically what FileHandler does. Theres so many places where we parse a jinja2-templated yaml and then pass to the filehandler, I figured it would be useful to make a class method out of it. I can remove it though.
There was a problem hiding this comment.
Yes. But, if there is an issue in the parsing, and one needs to print the resolved yaml, one will have to come in wxflow and do debugging.
Besides, passing only the necessary keys to the yaml path, rather than the entire task_config. would help in debugging.
src/wxflow/task.py
Outdated
| @@ -60,6 +62,9 @@ def __init__(self, config: Dict, *args, **kwargs): | |||
| self.runtime_config['previous_cycle'] = add_to_datetime(self.runtime_config.current_cycle, -to_timedelta(f"{self.config['assim_freq']}H")) | |||
| logger.debug(f"previous cycle: {self.runtime_config['previous_cycle']}") | |||
|
|
|||
| # Combine config and runtime_config into single task_config attribute-dictionary | |||
| self.task_config = AttrDict(**self.config, **self.runtime_config) | |||
There was a problem hiding this comment.
This will not force users to strictly use only self.task_config
I think we should make config and runtime_config internal (private) and only return self.task_config.
Change:
L35:
self._config = AttrDict(config)and L44
self._runtime_config = AttrDict()and then update the script to use these.
There was a problem hiding this comment.
Ok, good idea. I will have to make a companion PR for the Global Workflow though since config and runtime_config are referenced in several places.
There was a problem hiding this comment.
Yes. Unfortunately, it wasn't caught in the global-workflow at the time this happened, but this is necessary before we promote too many kinds of _config's.
src/wxflow/task.py
Outdated
| def extend_task_config(self, local_dict: Union[Dict, AttrDict]) -> None: | ||
| """ | ||
| Extend task_config attribute-dictionary with another dictionary | ||
| """ | ||
|
|
||
| self.task_config = AttrDict(**self.task_config, **local_dict) |
There was a problem hiding this comment.
This breaks provenance.
The intention is to create a task_config in the init. By allowing this, users can add/edit/remove items from task_config and it gets increasingly difficult to find out where what was added/removed.
As much as possible, we should extract only required pieces of information. If we turn these into global variables, it can get complicated quickly.
There was a problem hiding this comment.
Users are already doing this when they create task_config in places like the atm_analysis.py (https://github.com/NOAA-EMC/global-workflow/blob/205d0c2b13e2d7755cec75bf8c978ab20d453862/ush/python/pygfs/task/atm_analysis.py#L59) where config, runtime_config, and local_dict are combined into one task config. Even if I remove this method, there's nothing stopping someone from modifying task_config.
One possibility is to make task_config also private but accessible (read-only) using a public method, something like mytask.get_task_config()['foo']. In the case of the atm_analysis.py subclass __init__, one can just initialize it with like so:
super().__init__( AttrDict( **config, **local_dict ) )
and instead just create local_dict first before calling the above. Thus the user has one shot to add to task_config before it becomes read-only.
Description
This PR causes the the "task_config" member AttrDict object to be created in the init method of the "Task" class. Previously, "task_config" has been created separately, but all applications that utilize the "Task" class expect "task_config" to exist, so it might as well be created at initialization. Example of task_config creation: https://github.com/NOAA-EMC/global-workflow/blob/67b833e0c7bc390865d453588b4609a1a7ede981/ush/python/pygfs/task/atm_analysis.py#L59
This PR also adds two class methods to the "Task" class:
This PR is part of a broader refactoring of pygfs in the Global Workflow.
Type of change
How Has This Been Tested?
Checklist