Conversation
As a consequence ports become compulsory for outputs as well as inputs
it is implemented, use it!
- dynamic-complex: migrate cycles section from old list+port format to new dict format - DYAMOND_aiida: migrate cycles section from old list+port format to new dict format
GeigerJ2
left a comment
There was a problem hiding this comment.
OK, done with a first round of review. Have a bunch of comments, but nothing seems critical / blocking. Will have another round after the first ones are addressed
| port: str | None = None | ||
| class ConfigCycleTaskOutput(_NamedBaseModel): ... | ||
|
|
||
|
|
There was a problem hiding this comment.
Maybe we can use a type alias below to make the signature more readable?
type NamedModelListConverter[T: _NamedBaseModel] = Callable[
[list[T | str | dict] | None], list[T],
]then it would become:
def make_named_model_list_converter[NAMED_BASE_T: _NamedBaseModel](
cls: type[NAMED_BASE_T],
) -> NamedModelListConverter[NAMED_BASE_T]:
...| @@ -314,19 +307,33 @@ def convert_named_model_list( | |||
| return convert_named_model_list | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Here similarly:
type NamedModelDictConverter[T: _NamedBaseModel] = Callable[
[dict[str, list[T | str | dict]] | None],
dict[str, list[T]],
]then it would become:
def make_named_model_list_dict_converter[NAMED_BASE_T: _NamedBaseModel](
cls: type[NAMED_BASE_T],
) -> NamedModelDictConverter[NAMED_BASE_T]:
...There was a problem hiding this comment.
I'm actually not sure it helps for readability. Parts of the type are used below when defining the callable so that would be more hidden with type aliases.
| ) -> dict[str, list[NAMED_BASE_T]]: | ||
| if values is None: | ||
| return {} | ||
| named_model_list_converter = make_named_model_list_converter(cls) |
There was a problem hiding this comment.
Here, the inner converter is recreated on every call to convert_named_model_list_dict. Since cls doesn't change between invocations, the result of make_named_model_list_converter(cls) is always the same — it should be captured once in the closure:
# Before
def make_named_model_list_dict_converter[NAMED_BASE_T: _NamedBaseModel](
cls: type[NAMED_BASE_T],
) -> ...:
def convert_named_model_list_dict(values):
named_model_list_converter = make_named_model_list_converter(cls) # rebuilt every call
return {port: named_model_list_converter(data_list) for port, data_list in values.items()}
return convert_named_model_list_dict# After
def make_named_model_list_dict_converter[NAMED_BASE_T: _NamedBaseModel](
cls: type[NAMED_BASE_T],
) -> ...:
named_model_list_converter = make_named_model_list_converter(cls) # built once
def convert_named_model_list_dict(values):
if values is None:
return {}
return {port: named_model_list_converter(data_list) for port, data_list in values.items()}
return convert_named_model_list_dictThere was a problem hiding this comment.
Hum... cls does change. make_named_model_list_dict_converter is called once with ConfigCycleTaskInput and once with ConfigCycleTaskOutput
| outputs: | ||
| - stored_data_1 | ||
| - atmo_log | ||
| archive: [stored_data_1] |
There was a problem hiding this comment.
Did atmo_log got dropped intentionally?
| archive: [stored_data_1] | |
| archive: [stored_data_1, atmo_log] |
If not, should also be added again to data/generated.
There was a problem hiding this comment.
Yes, it did not make sense in this context. I need to add an output port with dynamical file name for all model logs of an icon task but that requires first the core.Task.components level I'm working on.
Invert order of port and data in config file.
As a consequence ports become compulsory for outputs as well as inputs.