Some adaptations to workflows #9994

yngve-sk · 2025-02-06T11:06:13Z

Issue
Resolves equinor/semeio#685

Add support for defaulted kwargs from workflows

codspeed-hq · 2025-02-06T11:34:17Z

CodSpeed Performance Report

Merging #9994 will improve performances by 13.37%

_{Comparing yngve-sk:25.02.05.internal-workflows-pass-some-stuff (9d1a66e) with main (3f4a93e)}

Summary

⚡ 1 improvements
✅ 24 untouched benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`test_direct_dark_performance_with_storage[gen_x: 20, sum_x: 20 reals: 10-gen_data_with_obs-get_record_observations]`	2.5 ms	2.2 ms	+13.37%

oyvindeide

Looks good, I have some minor questions.

oyvindeide · 2025-02-09T20:28:10Z

src/ert/cli/workflow.py

+    runner = WorkflowRunner(
+        workflow=workflow,
+        storage=storage,
+        ert_config=ert_config,


I guess we should remove this as it is not available in run models, where most of these are run from?

oyvindeide · 2025-02-09T20:29:11Z

src/ert/config/ert_script.py

+                if is_using_wf_args_fixture
+                else [*workflow_args, *fixture_args]
+            )
+            if not positional_args and not use_kwargs:


This we could keep this as it used to be? I dont think kwargs were really supported before, they just ended up being workflow_args in a list?

oyvindeide · 2025-02-09T20:31:48Z

src/ert/workflow_runner.py

@@ -107,14 +107,16 @@ class WorkflowRunner:
    def __init__(
        self,
        workflow: Workflow,
+        config_file: str | None = None,


Also commented in semeio, could potentially give reports dir, we do something similar for the update report, that has a folder it writes to.

oyvindeide · 2025-02-09T20:32:03Z

src/ert/workflow_runner.py

        storage: Storage | None = None,
        ensemble: Ensemble | None = None,
        ert_config: ErtConfig | None = None,
    ) -> None:
        self.__workflow = workflow
        self.storage = storage
        self.ensemble = ensemble
-        self.ert_config = ert_config
+        self.ert_config = ert_config  # Should eventually be removed


Remove now?

jonathan-eq · 2025-02-11T09:30:54Z

src/ert/config/ert_script.py

-        func_args: MappingProxyType[str, inspect.Parameter],
-        fixtures: dict[str, Fixtures],
+        func_args: dict[str, inspect.Parameter],
+        fixtures: dict[WorkflowFixtures, Any],


I am not entirely getting this. If fixtures is a dict, where the keys can only be any of the WorkflowFixtures enum values; shouldnt WorkflowFixtures be a dataclass and we would use fixtures: WorkflowFixtures instead of fixtures: dict[WorkflowFixtures, Any]?
Why is the fixture value Any?

Hmm it could be set as a dataclass I think, but eventually the fixtures are passed in as arguments, and we also give them in a very "partial" way. We can do this in dataclasses by omitting stuff etc etc, but there we would have lots of default'ed None values. In some cases we want an error if a workflow requests an argument that is not a fixture, nor a provided kwarg. If we had a dataclass, we would have to know whether the None in the fixture means an explicit None from outside, or a truly "missing" argument.

What if we have it as a TypedDict and mark the types with NotRequired?

That is probably better, testing...

jonathan-eq · 2025-02-11T09:34:22Z

src/ert/config/ert_script.py

-        fixtures: dict[str, Fixtures],
+        func_args: dict[str, inspect.Parameter],
+        fixtures: dict[WorkflowFixtures, Any],
+        kwargs: dict[str, Any],
    ) -> list[Any]:
        arguments = []
        errors = []
        for val in func_args:


What is val here? Is it the required argument?

Yep it is required, and we mark it as an error if val is not in the fixtures, or explicit kwargs the run fn is invoked with. So func_args here is the arguments of the run function of the workflow, and here we are creating the arguments to call the run function with, by inserting the values for corresponding fixtures. So the whole purpose of the fixtures is to substitute here.

Ahhh, okay! That makes sense! Very good 👍

jonathan-eq · 2025-02-11T10:02:25Z

src/ert/run_models/base_run_model.py

+        self.run_workflows(
+            HookRuntime.POST_SIMULATION,
+            fixtures={
+                WorkflowFixtures.storage: self._storage,
+                WorkflowFixtures.ensemble: ensemble,
+                WorkflowFixtures.reports_dir: self.reports_dir(
+                    ensemble_name=ensemble.name
+                ),
+                WorkflowFixtures.random_seed: self.random_seed,
+                WorkflowFixtures.run_paths: self.run_paths,
+            },
+        )


Is this and the the self.run_workflows above it the same, except for the ensemble maybe changing? Maybe we can move most of it into the run_workflows method?

I think it makes most sense to keep the fixtures as an arg to run_workflows, since it is invoked from other places than BaseRunModel at different points/hooks in the ERT execution, and it makes it clear at the callsites which fixtures are given to the workflows.

Okay, good!

jonathan-eq

Very good :) See some minor questions 🔍

jonathan-eq · 2025-02-11T13:09:39Z

src/ert/config/workflow_fixtures.py

+    from ert.storage import Ensemble, Storage
+
+
+class WorkflowFixtures(TypedDict):


I think you can omit marking all keys with NotRequired if you change class WorkflowFixtures(TypedDict) to class WorkflowFixtures(TypedDict, total=False) https://peps.python.org/pep-0589/#totality

jonathan-eq · 2025-02-11T13:10:39Z

src/ert/gui/tools/plugins/plugin.py

        func_args = inspect.signature(script.getArguments).parameters
-        arguments = script.insert_fixtures(func_args, fixtures)
+        arguments = script.insert_fixtures(dict(func_args), fixtures, {})

        # Part of deprecation


leftover comment

^the part of deprecation line

jonathan-eq · 2025-02-11T13:17:44Z

src/ert/workflow_runner.py

 if TYPE_CHECKING:
-    from ert.storage import Ensemble, Storage
+    pass


Lets remove this type_checking :)

jonathan-eq · 2025-02-11T13:20:29Z

tests/ert/unit_tests/config/test_ert_plugin.py

        ["1", "2", "3"],
        fixture_mock,
    )


-def test_plugin_with_default_arguments(capsys):
+def test_plugin_with_fixtures_and_enough_arguments_positional():


I think test_plugin_with_fixtures_and_enough_positional_arguments would read a tad better

jonathan-eq · 2025-02-11T13:21:14Z

tests/ert/unit_tests/config/test_ert_plugin.py

    )


+def test_plugin_with_default_arguments(capsys):


Is this test using capsys?

Removed it in separate commit

Co-authored-by: Jonathan Karlsen <[email protected]>

jonathan-eq · 2025-02-11T15:53:28Z

There seems to be a GUI test hanging now 🤔

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 2 times, most recently from e38fc26 to 16d8860 Compare February 6, 2025 11:11

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 3 times, most recently from a1fc9a1 to ab916da Compare February 6, 2025 13:40

oyvindeide reviewed Feb 9, 2025

View reviewed changes

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 8 times, most recently from a60c42c to 6e47da3 Compare February 11, 2025 08:55

jonathan-eq self-requested a review February 11, 2025 09:13

jonathan-eq reviewed Feb 11, 2025

View reviewed changes

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 3 times, most recently from 87c7ada to 0aeee68 Compare February 11, 2025 13:11

jonathan-eq requested changes Feb 11, 2025

View reviewed changes

jonathan-eq mentioned this pull request Feb 11, 2025

Fix workflows running from gui's tools panel not working #9954

Closed

9 tasks

yngve-sk and others added 6 commits February 11, 2025 15:17

Pass more workflow fixtures explicitly

157a386

Co-authored-by: Jonathan Karlsen <[email protected]>

Remove "parent" fixture to workflow from gui

71f14a5

Stop using ert_config for workflows

032d20c

Co-authored-by: Jonathan Karlsen <[email protected]>

Support kwargs for workflows

048d655

Remove weird line from csv_export.py

7d402ae

Co-authored-by: Jonathan Karlsen <[email protected]>

rm unnecessary use of capsys from test

9d1a66e

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 0aeee68 to 9d1a66e Compare February 11, 2025 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some adaptations to workflows #9994

Some adaptations to workflows #9994

yngve-sk commented Feb 6, 2025

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading

oyvindeide left a comment

oyvindeide Feb 9, 2025

oyvindeide Feb 9, 2025

oyvindeide Feb 9, 2025

oyvindeide Feb 9, 2025

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025 •

edited

Loading

jonathan-eq Feb 11, 2025

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025

jonathan-eq Feb 11, 2025

jonathan-eq left a comment

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025

jonathan-eq Feb 11, 2025

jonathan-eq Feb 11, 2025

jonathan-eq Feb 11, 2025

jonathan-eq Feb 11, 2025

jonathan-eq Feb 11, 2025

yngve-sk Feb 11, 2025

jonathan-eq commented Feb 11, 2025

		from ert.storage import Ensemble, Storage


		class WorkflowFixtures(TypedDict):

Some adaptations to workflows #9994

Are you sure you want to change the base?

Some adaptations to workflows #9994

Conversation

yngve-sk commented Feb 6, 2025

codspeed-hq bot commented Feb 6, 2025 • edited Loading

Merging #9994 will improve performances by 13.37%

Summary

Benchmarks breakdown

oyvindeide 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yngve-sk Feb 11, 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

jonathan-eq 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

Choose a reason for hiding this comment

jonathan-eq commented Feb 11, 2025

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading

yngve-sk Feb 11, 2025 •

edited

Loading