Tests and canonicalization for ConfigWorkflow #89

DropD · 2025-01-14T15:44:26Z

First step towards clarifying and testing the data layout from which we can reliably build workflows.

Problem

See #88. This covers steps 1-8 for ConfigWorkflow

Changes

Doctests
- enabled by default (required fixing some pretty_printer doctests)
- Doctests for ConfigWorkflow, outlining the smallest possible valid yaml snippet
Unit tests
- Check that we can build a CanonicalWorkflow from code as well as from the mimimal ConfigWorkflow instance
- Check that we can use the intended load_workflow_config method to load the minimal yaml snippet
- Check that we can convert the minimum valid CanonicalWorkflow into a core.Workflow

DropD · 2025-01-15T09:14:37Z

src/sirocco/parsing/_yaml_data_models.py

+            >>> empty_wf = ConfigWorkflow(cycles=[], tasks=[], data={})
+
+    """
+


Question: are we ok with this yaml snippet parsing without validation errors?

If so, should it fail with a clear message down the line or should it create a viable WorkGraph, which runs nothing?

we agreed in the meeting to produce an error, maybe it is easier for merging to do this in a subsequent PR

once I merge #90 into this, the error would be raised during canonicalization

DropD · 2025-01-15T09:16:36Z

tests/unit_tests/core/test_workflow.py

+
+    pretty_print.PrettyPrinter().format(testee)
+
+    assert testee.name is None


Question: What does it mean to have a core.Workflow with name=None, rootdir=None?

Realistically this can not happen in normal usage, (as tested in test_yaml_data_models.py). However, downstream code has to deal with the possibility or negate the help of static type checking.

So we need to check if they are not None in the core.Workflow right? Should we make an issue to keep in mind?

Eventually, for core objects, the name and rootdir are compulsory but the reason for None possibility in the user provided information is different for both of them.

name can be None and then defaults in the core object to what is derived from the yaml config path.

rootdir should actually NOT be given by the user and only inferred from the yaml config path.

So if you create a canonical class:

name would stay str | None in ConfigWorkflow defaulting to None and be non default str in the canonical class

rootdir would belong to the canonical class as non default str and disappear from ConfigWorkflow

Ok, I will adapt this in #90

I will need to think about how to adapt the canonicalize function to allow passing in additional information though, because the additional information will be different per class if any (not sure if singledispatch is still the right match then).

DropD · 2025-01-15T09:19:32Z

tests/unit_tests/parsing/test_yaml_data_models.py

+def test_workflow_test_internal_dicts():
+    testee = models.ConfigWorkflow(
+        cycles=[],
+        tasks=[{"some_task": {"plugin": "shell"}}],


Question: is it a problem, that we can not currently (for testing purposes) pass actual ConfigXyzTask instances into the constructor of ConfigWorkflow, but have to go through this dictionary form?

I think yes. As you pointed out in the last meeting, we need to decouple the components for effective testing so we need to test YAML <-> CONFIG and CONFIG <-> GRAPH separately which requires a creation of the config objects without a yaml string.

What do you think of the drafted solution in #90, splitting into a "canonical" and a "yaml-parsed" class? Note that
a) Some of this would be alleviated by idempotent validators, which that approach would introduce gradually
b) The full benefits would only kick in, once all the config classes have undergone the same process. However, testability will increase incrementally.

made a comment there about the canonical workflow

I agree with the creation of canonical classes to remove the type ambiguities. Same as rootdir in WorkFlowConfig.

agoscinski

The questions you point out would need some further changes to be implement but maybe we implement them in another PR to avoid merge conflicts and focus on the canonicalization? I approve already.

agoscinski · 2025-01-17T10:38:58Z

src/sirocco/parsing/_yaml_data_models.py

+            >>> empty_wf = ConfigWorkflow(cycles=[], tasks=[], data={})
+
+    """
+


we agreed in the meeting to produce an error, maybe it is easier for merging to do this in a subsequent PR

agoscinski · 2025-01-17T10:41:56Z

tests/unit_tests/core/test_workflow.py

+
+    pretty_print.PrettyPrinter().format(testee)
+
+    assert testee.name is None


So we need to check if they are not None in the core.Workflow right? Should we make an issue to keep in mind?

* finalize ConfigWorkflow canonicalization * improve type hints

src/sirocco/parsing/_yaml_data_models.py

DropD added 2 commits January 14, 2025 16:12

add tests related to ConfigWorkflow

954869b

avoid doctests collecting scripts in tests/cases

81c8979

DropD requested a review from leclairm January 15, 2025 09:06

DropD commented Jan 15, 2025

View reviewed changes

DropD changed the title ~~add tests related to ConfigWorkflow~~ Tests and questions related to ConfigWorkflow Jan 15, 2025

DropD requested a review from agoscinski January 16, 2025 09:09

finalize ConfigWorkflow canonicalization, making assumptions

f5669f1

agoscinski approved these changes Jan 17, 2025

View reviewed changes

leclairm approved these changes Jan 17, 2025

View reviewed changes

DropD added 2 commits January 17, 2025 15:52

improve type hints

63d6801

Example canonicalization for ConfigWorkflow (#90)

bec3583

* finalize ConfigWorkflow canonicalization * improve type hints

DropD changed the title ~~Tests and questions related to ConfigWorkflow~~ Tests and questions canonicalization for ConfigWorkflow Jan 17, 2025

DropD added 4 commits January 17, 2025 16:27

cleanup and review comments

05ed3aa

Merge branch 'main' into improve-config-workflow

d3c0621

minor improvements

65c84b7

Merge branch 'improve-config-workflow' into test-config-workflow

2e3bcf5

agoscinski added this to the Testing components (parser, core, workgraph) milestone Jan 20, 2025

cleanup after merging

21302d1

DropD changed the title ~~Tests and questions canonicalization for ConfigWorkflow~~ Tests and canonicalization for ConfigWorkflow Jan 20, 2025

leclairm reviewed Jan 20, 2025

View reviewed changes

src/sirocco/parsing/_yaml_data_models.py Outdated Show resolved Hide resolved

update arg name in canonicalize_workflow

dbe4597

DropD mentioned this pull request Jan 21, 2025

Move the canonical class to parsing and core modules #100

Merged

DropD merged commit 2c0ee10 into main Jan 21, 2025
3 checks passed

DropD deleted the test-config-workflow branch January 21, 2025 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests and canonicalization for ConfigWorkflow #89

Tests and canonicalization for ConfigWorkflow #89

DropD commented Jan 14, 2025 •

edited

Loading

DropD Jan 15, 2025

agoscinski Jan 17, 2025

DropD Jan 17, 2025

DropD Jan 15, 2025

DropD Jan 15, 2025

agoscinski Jan 17, 2025

leclairm Jan 17, 2025 •

edited

Loading

DropD Jan 17, 2025

DropD Jan 17, 2025

DropD Jan 15, 2025

agoscinski Jan 16, 2025

DropD Jan 16, 2025

agoscinski Jan 17, 2025

leclairm Jan 17, 2025

agoscinski left a comment •

edited

Loading

agoscinski Jan 17, 2025

agoscinski Jan 17, 2025

		>>> empty_wf = ConfigWorkflow(cycles=[], tasks=[], data={})

		"""


		pretty_print.PrettyPrinter().format(testee)

		assert testee.name is None

Tests and canonicalization for ConfigWorkflow #89

Tests and canonicalization for ConfigWorkflow #89

Conversation

DropD commented Jan 14, 2025 • edited Loading

Problem

Changes

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

leclairm Jan 17, 2025 • edited Loading

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

agoscinski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DropD commented Jan 14, 2025 •

edited

Loading

leclairm Jan 17, 2025 •

edited

Loading

agoscinski left a comment •

edited

Loading