Some adaptations to workflows #9994

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

Issue
Resolves equinor/semeio#685
Resolves #9389

Add support for defaulted kwargs from workflows

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

CodSpeed Performance Report

Merging #9994 will not alter performance

_{Comparing yngve-sk:25.02.05.internal-workflows-pass-some-stuff (6d042ec) with main (20dff0f)}

Summary

✅ 25 untouched benchmarks

oyvindeide

Looks good, I have some minor questions.

src/ert/cli/workflow.py

src/ert/config/ert_script.py

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.

src/ert/workflow_runner.py

src/ert/config/ert_script.py

src/ert/run_models/base_run_model.py

jonathan-eq

Very good :) See some minor questions 🔍

src/ert/config/workflow_fixtures.py

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

src/ert/workflow_runner.py

tests/ert/unit_tests/config/test_ert_plugin.py

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

There seems to be a GUI test hanging now 🤔

src/ert/config/workflow_fixtures.py

jonathan-eq · 2025-02-13T06:46:25Z

Will this resolve #9389?

yngve-sk · 2025-02-13T07:31:41Z

Will this resolve #9389?

I think so yes, as it will no longer need the ertconfig

jonathan-eq · 2025-02-13T07:43:42Z

Looks good to me, @oyvindeide should have a look at it too 🚀

oyvindeide

Looks good, but can we drop kwargs support in this PR and still fix the bug? Then feel free to suggest it in a different PR, but think that requires some additional tests.

oyvindeide · 2025-02-14T10:44:03Z

src/ert/config/ert_script.py

-    @deprecated("Use fixtures to the run function instead")
-    def ert(self) -> ErtConfig | None:
-        logger.info(f"Accessing EnKFMain from workflow: {self.__class__.__name__}")
-        return self._ert
-
-    @property
-    def ensemble(self) -> Ensemble | None:
-        warnings.warn(
-            "The ensemble property is deprecated, use the fixture to the run function instead",
-            DeprecationWarning,
-            stacklevel=1,
-        )
-        logger.info(f"Accessing ensemble from workflow: {self.__class__.__name__}")
-        return self._ensemble
-
-    @property
-    def storage(self) -> Storage | None:
-        warnings.warn(
-            "The storage property is deprecated, use the fixture to the run function instead",
-            DeprecationWarning,
-            stacklevel=1,
-        )
-        logger.info(f"Accessing storage from workflow: {self.__class__.__name__}")
-        return self._storage
-


These deprecations should stay, but can raise an error.

src/ert/libres_facade.py

oyvindeide · 2025-02-14T10:50:43Z

src/ert/run_models/base_run_model.py

@@ -181,13 +182,20 @@ def __init__(
        self.start_iteration = start_iteration
        self.restart = False

+    def reports_dir(self, ensemble_name: str) -> str:


Should use log_path

Hmm I see, this was just replicating old behavior, but log_path is the update_log_path, but workflows are not necessarily related to the update? Or is the update_log_path now just the "output" for non-storage, but permanent files pertaining to an ERT run?

Yes, the latter, it is just the output path for things related to that experiment.

tests/ert/unit_tests/plugins/test_export_misfit.py

tests/ert/unit_tests/workflow_runner/test_workflow_runner.py

src/ert/config/ert_script.py

oyvindeide · 2025-02-14T10:58:28Z

src/ert/config/ert_script.py

+            use_kwargs = {
+                k: (kwargs or {}).get(k, default_value)
+                for k, default_value in ({**kwargs_defaults, **kwargs}).items()


Not sure kwarg support is worth the extra complication, can we do without it?

Should be in another PR unless it is needed to fix the bug, think we might need some additional tests for that.

We can. I will use workflow_args and move the defaulting to not be in the function signature of ahmanalysis, as for the test that modifies/overrides alpha for the update settings I will edit it in the ert config before doing .run_ertscript.

Sounds good, yes, can remove the defaulting in the AHMAnalysis job

oyvindeide · 2025-02-14T13:33:55Z

src/ert/config/ert_script.py

@@ -71,36 +68,36 @@ def stderrdata(self) -> str:
            self._stderrdata = self._stderrdata.decode()
        return self._stderrdata

+    def isCancelled(self) -> bool:


oyvindeide · 2025-02-14T13:34:56Z

src/ert/config/ert_script.py

@@ -114,7 +111,7 @@ def initializeAndRun(
        self,
        argument_types: list[type[Any]],
        argument_values: list[str],
-        fixtures: dict[str, Any] | None = None,
+        fixtures: WorkflowFixtures | None = None,


Typing problem. Is this hit by tests? In case it is actually incompatible.

oyvindeide · 2025-02-14T13:35:18Z

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

        """
        Returns a list of arguments. Either from GUI or from arbitrary code.
        If the user for example cancels in the GUI a CancelPluginException is raised.
        """
        script = self.__loadPlugin()
        fixtures["parent"] = self.__parent_window
        func_args = inspect.signature(script.getArguments).parameters
-        arguments = script.insert_fixtures(func_args, fixtures)
+        arguments = script.insert_fixtures(dict(func_args), fixtures, {})


Seems like more than a typing problem?

Yeah removing, empty placeholder for kwargs

oyvindeide · 2025-02-14T13:35:49Z

src/ert/gui/tools/workflows/run_workflow_widget.py

+                "storage": self.storage,
+                "random_seed": self.config.random_seed,
+                "reports_dir": str(
+                    self.storage.path.parent


log_path here too

oyvindeide · 2025-02-14T13:35:59Z

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

+                    "storage": self.storage,
+                    "random_seed": ert_config.random_seed,
+                    "reports_dir": str(
+                        self.storage.path.parent


oyvindeide · 2025-02-14T13:36:22Z

src/ert/libres_facade.py

+                "ensemble": ensemble,
+                "reports_dir": (
+                    storage.path.parent
+                    / "reports"


oyvindeide

LGTM! Just some minor comments.

oyvindeide · 2025-02-17T07:12:57Z

src/ert/run_models/base_run_model.py

@@ -213,13 +216,15 @@ def api(self) -> BaseRunModelAPI:
            cancel=self.cancel,
        )

+    def reports_dir(self, experiment_name: str) -> str:
+        return str(self._log_path / experiment_name)


Is it ok that this does not exist?

Don't think so, assuming there is always a log_path in every BaseRunModel as it comes from the ERT config, or is otherwise explicitly passed in.

oyvindeide · 2025-02-17T07:15:46Z

tests/ert/unit_tests/cli/test_cli_workflow.py

@@ -21,4 +21,4 @@ def test_executing_workflow(storage):
        rc = ErtConfig.from_file(config_file)
        args = Namespace(name="test_wf")
        execute_workflow(rc, storage, args.name)
-        assert os.path.isfile(".ert_runpath_list")
+        assert os.path.isfile("test_workflow_output.csv")


Suggest parameterizing this test if it is fast so both workflows are run.

oyvindeide · 2025-02-17T07:16:58Z

src/ert/config/ert_script.py

        logger.info(f"Accessing EnKFMain from workflow: {self.__class__.__name__}")
-        return self._ert
+        raise KeyError("Attribute 'ert' is deprecated, use fixtures instead.")


Nitpick, but this should be an AttributeError

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

jonathan-eq

Very good

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

src/ert/config/ert_script.py Outdated Show resolved Hide resolved

jonathan-eq reviewed Feb 11, 2025

View reviewed changes

src/ert/config/ert_script.py Show resolved Hide resolved

jonathan-eq reviewed Feb 11, 2025

View reviewed changes

src/ert/run_models/base_run_model.py Outdated Show resolved Hide resolved

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 force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 0aeee68 to 9d1a66e Compare February 11, 2025 14:20

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 2 times, most recently from f094ca8 to 1e668d0 Compare February 12, 2025 11:20

jonathan-eq reviewed Feb 12, 2025

View reviewed changes

src/ert/config/workflow_fixtures.py Outdated Show resolved Hide resolved

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 1e668d0 to 563aa3f Compare February 12, 2025 13:22

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 563aa3f to aeccdd7 Compare February 12, 2025 13:32

jonathan-eq added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Feb 13, 2025

oyvindeide reviewed Feb 14, 2025

View reviewed changes

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from aeccdd7 to 6ad5a19 Compare February 14, 2025 11:40

oyvindeide reviewed Feb 14, 2025

View reviewed changes

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch 3 times, most recently from b849ab8 to ab80df4 Compare February 14, 2025 14:53

oyvindeide approved these changes Feb 17, 2025

View reviewed changes

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from ab80df4 to 3e43f4e Compare February 17, 2025 12:06

yngve-sk and others added 5 commits February 19, 2025 08:53

Pass more workflow fixtures explicitly

a8e1b07

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

Stop using ert_config for workflows

5f6b547

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

Remove weird line from csv_export.py

53f68ba

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

Remove unnecessary poly case usage

d7abc61

Set ensemble=None on wf cli run

6d042ec

yngve-sk force-pushed the 25.02.05.internal-workflows-pass-some-stuff branch from 3e43f4e to 6d042ec Compare February 19, 2025 07:56

jonathan-eq approved these changes Feb 19, 2025

View reviewed changes

yngve-sk merged commit 5a86d20 into equinor:main Feb 19, 2025
26 of 27 checks passed

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 •

edited

Loading

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading

oyvindeide left a comment

oyvindeide Feb 9, 2025

jonathan-eq left a comment

jonathan-eq commented Feb 11, 2025

jonathan-eq commented Feb 13, 2025

yngve-sk commented Feb 13, 2025

jonathan-eq commented Feb 13, 2025

oyvindeide left a comment

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

yngve-sk Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

yngve-sk Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

yngve-sk Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide Feb 14, 2025

oyvindeide left a comment

oyvindeide Feb 17, 2025

yngve-sk Feb 17, 2025

oyvindeide Feb 17, 2025

oyvindeide Feb 17, 2025

jonathan-eq left a comment

Some adaptations to workflows #9994

Some adaptations to workflows #9994

Conversation

yngve-sk commented Feb 6, 2025 • edited Loading

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

Merging #9994 will not alter performance

Summary

oyvindeide left a 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

jonathan-eq commented Feb 11, 2025

jonathan-eq commented Feb 13, 2025

yngve-sk commented Feb 13, 2025

jonathan-eq commented Feb 13, 2025

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

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

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

jonathan-eq left a comment

Choose a reason for hiding this comment

yngve-sk commented Feb 6, 2025 •

edited

Loading

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading