Refactor dates handling #121

leclairm · 2025-02-06T13:40:07Z

Fix #119, #97 and incidentally #108

Short summary: essentially remove as many None as possible from date related attributes in order to make the code more understandable.

Methods

Insert dedicated classes to test types instead of testing for None
Move some logic to the yaml models although it was already argued against once
Introduce cycling and target_cycle keywords in the config format

Parts to treat

Previous _WhenBasemodel
ConfigCycle

also rename target_date -> target_cycle

DropD

I like the design, there are some things to still address though.

DropD · 2025-02-12T10:59:44Z

src/sirocco/parsing/yaml_data_models.py

+    @field_validator("type")
+    @classmethod
+    def is_file_or_dir(cls, value: str) -> str:
+        """."""
+        valid_types = ("file", "dir")
+        if value not in valid_types:
+            msg = f"Must be one of {valid_types}"
+            raise ValueError(msg)
+        return value
+


This validator is no longer useful and was removed in #120 .

Thanks, this went unnoticed during rebasing because I changed the name of the file and #120 got merged after.

DropD · 2025-02-12T11:03:33Z

src/sirocco/parsing/yaml_data_models.py

-            msg = f"For cycle {self.name!r} the period {self.period!r} is negative or zero."
-            raise ValueError(msg)
-        return self
+    cycling: Annotated[Cycling, BeforeValidator(select_cycling)] = OneOff()


I think this is a great way of selecting which subclass to parse to 👍.

DropD · 2025-02-12T11:13:25Z

src/sirocco/pretty_print.py

+        ...             cycle_point=DateCyclePoint(
+        ...                 start_date=datetime(1000, 1, 1),
+        ...                 stop_date=datetime(1000, 1, 2),
+        ...                 begin_date=datetime(1000, 1, 1),
+        ...                 end_date=datetime(1000, 1, 2),
+        ...             ),


It just occurred to me while reading this that I didn't see an explanation anywhere for the meaning of start_date / stop_date vs begin_date / end_date. I assume one of these pairs refers to the overall experiment and one to the cycle?

Yes, exactly. It's missing some docstring. I followed the ICON convention for overall start/stop and then chose the other available pair, begin/end, for the current cycle. Currently, we set ICON start and stop at each cycle so I needed this info to be carried along. And it's very likely that other tasks will want to access begin/end dates somehow (#61 and #85)

src/sirocco/parsing/cycling.py

DropD

Looks good now

GeigerJ2

Great work, @leclairm, thanks a lot! Both, code and YAML files now read much cleaner. LGTM (~~looks good to me~~ let's get this merged)!

src/sirocco/parsing/cycling.py

GeigerJ2 · 2025-02-13T11:34:52Z

src/sirocco/parsing/cycling.py

+from __future__ import annotations
+
+from abc import ABC, abstractmethod
+from collections.abc import Iterator  # noqa: TCH003 needed for pydantic


Rules should be TC002 and TC003, no?
https://docs.astral.sh/ruff/rules/typing-only-third-party-import/
Possibly, we could also add them here if they are relevant in more places as we use pydantic throughout:

Sirocco/pyproject.toml

Lines 71 to 75 in bb303d0

[tool.ruff.lint]

ignore = [

"TID252", # prefer relative import over absolute

"TRY003", # write custom error messages for formatting

]

But not sure yet 🤔

GeigerJ2 · 2025-02-13T12:34:55Z

src/sirocco/parsing/yaml_data_models.py

+        case Cycling():
+            return spec
+        case dict():
+            if spec.keys() != {"start_date", "stop_date", "period"}:


Suggested change

if spec.keys() != {"start_date", "stop_date", "period"}:

if set(spec.keys()) != {"start_date", "stop_date", "period"}:

Maybe explicitly convert to a set here, so we compare the same objects, and not set and dict_keys?

GeigerJ2 · 2025-02-13T13:09:49Z

src/sirocco/parsing/yaml_data_models.py

+            return spec
+        case dict():
+            if not all(k in ("at", "before", "after") for k in spec):
+                msg = "when keys can only be 'at', 'before' or 'after'"


Suggested change

msg = "when keys can only be 'at', 'before' or 'after'"

msg = "'when' keys can only be 'at', 'before' or 'after'"

GeigerJ2 · 2025-02-13T13:22:12Z

src/sirocco/parsing/yaml_data_models.py

+        case TargetCycle():
+            return spec
+        case dict():
+            if tuple(spec.keys()) not in (("date",), ("lag",)):


Suggested change

if tuple(spec.keys()) not in (("date",), ("lag",)):

if not (("date" in spec) ^ ("lag" in spec)):

Using XOR? I suppose there can be no other keys?

It's actually making sure there are no other keys.

GeigerJ2 · 2025-02-13T13:27:07Z

src/sirocco/parsing/yaml_data_models.py

+    target_cycle: Annotated[TargetCycle, BeforeValidator(select_target_cycle)] = NoTargetCycle()
+    when: Annotated[When, BeforeValidator(select_when)] = AnyWhen()
+    parameters: Annotated[dict[str, Literal["all", "single"]], BeforeValidator(check_parameters_spec)] = {}


Very nice to read :3

GeigerJ2 · 2025-02-13T13:37:18Z

src/sirocco/parsing/yaml_data_models.py

@@ -613,6 +566,20 @@ def get_plugin_from_named_base_model(
 ]


+def check_parameters_lists(data: Any) -> dict[str, list]:


Maybe we can move all the pydantic validators that are now defined as standalone functions into a dedicated file validators.py? Kind of separation of concerns (ofc I know, though, that the validators are connected to the Data models). Just proposing this bc I found it a bit difficult to go through this file, switching between BaseModel derived classes that define our data models, and functions that do some actions.

I would like that to happen, but in a different PR to avoid conflicts right now.

Yes, I had the same thought while implementing. I even started by putting some validators in _utlis.py but didn't want to go all the way. Actually _utils.py is only validators.

This reverts commit d54ad9c.

This was linked to issues Feb 6, 2025

possibility of None for date related variables #119

Closed

Specify type of parameters more explicit #108

Closed

leclairm changed the title ~~None dates refactor~~ Refactor dates handling Feb 6, 2025

leclairm linked an issue Feb 6, 2025 that may be closed by this pull request

confusing start and end dates for cycles, tasks and ICON #97

Closed

leclairm marked this pull request as ready for review February 10, 2025 21:54

leclairm requested review from DropD and agoscinski February 10, 2025 21:58

leclairm added 12 commits February 11, 2025 21:18

ref: add dedicated classes for when possibilities

8e884dc

ref: move logic to models using ABCs

78fa9a4

fix: hatch fmt

2d71eaa

fix: leftover from TypeAlias strategy

43874b7

fix: better parameter type for TargetNodesBaseModel (#108)

e29d628

ref: end_date -> stop_date

e13426b

wip: add new modules for date realted objects

0a7c075

fix: tests and ICON task

fd5029a

also rename target_date -> target_cycle

fix: missing files ...

c9dfd24

ref: move pydantic validation to dedicated modules

8fa3370

fix: useless error messages

c5022b5

ref: (minor) swap if logic

561a5a8

leclairm force-pushed the none-dates-refactor branch from bc2a275 to 561a5a8 Compare February 11, 2025 20:22

DropD requested changes Feb 12, 2025

View reviewed changes

leclairm added 3 commits February 12, 2025 22:13

del: useless test with new yaml models

f459773

fix: address review

ad3a7e3

fix: hatch fmt

345fecd

leclairm requested a review from DropD February 12, 2025 22:03

DropD approved these changes Feb 13, 2025

View reviewed changes

ref: begin/end => chunk_start/chunk_stop

ed150b1

DropD mentioned this pull request Feb 13, 2025

Improve yaml models: ConfigTasks #122

Merged

GeigerJ2 approved these changes Feb 13, 2025

View reviewed changes

DropD merged commit d54ad9c into main Feb 13, 2025
7 checks passed

DropD deleted the none-dates-refactor branch February 13, 2025 13:51

DropD restored the none-dates-refactor branch February 13, 2025 13:54

DropD added a commit that referenced this pull request Feb 13, 2025

Revert "Refactor dates handling (#121)"

b7b362c

This reverts commit d54ad9c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dates handling #121

Refactor dates handling #121

leclairm commented Feb 6, 2025 •

edited

Loading

DropD left a comment

DropD Feb 12, 2025

leclairm Feb 12, 2025

DropD Feb 12, 2025

DropD Feb 12, 2025

leclairm Feb 12, 2025

DropD left a comment

GeigerJ2 left a comment

GeigerJ2 Feb 13, 2025

GeigerJ2 Feb 13, 2025

GeigerJ2 Feb 13, 2025

GeigerJ2 Feb 13, 2025

leclairm Feb 13, 2025

GeigerJ2 Feb 13, 2025

GeigerJ2 Feb 13, 2025

DropD Feb 13, 2025 •

edited

Loading

leclairm Feb 13, 2025 •

edited

Loading

	[tool.ruff.lint]
	ignore = [
	"TID252", # prefer relative import over absolute
	"TRY003", # write custom error messages for formatting
	]

	if spec.keys() != {"start_date", "stop_date", "period"}:
	if set(spec.keys()) != {"start_date", "stop_date", "period"}:

	msg = "when keys can only be 'at', 'before' or 'after'"
	msg = "'when' keys can only be 'at', 'before' or 'after'"

	if tuple(spec.keys()) not in (("date",), ("lag",)):
	if not (("date" in spec) ^ ("lag" in spec)):

		@@ -613,6 +566,20 @@ def get_plugin_from_named_base_model(
		]


		def check_parameters_lists(data: Any) -> dict[str, list]:

Refactor dates handling #121

Refactor dates handling #121

Conversation

leclairm commented Feb 6, 2025 • edited Loading

Methods

Parts to treat

DropD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DropD left a comment

Choose a reason for hiding this comment

GeigerJ2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DropD Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

leclairm Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

leclairm commented Feb 6, 2025 •

edited

Loading

DropD Feb 13, 2025 •

edited

Loading

leclairm Feb 13, 2025 •

edited

Loading