Add forward models as plugins #7743

yngve-sk · 2024-04-23T13:00:10Z

Not about preinstalled forward model steps "bleeding" into other tests causing failure
The preinstalled forward model steps are added as a global singleton ClassVar to ErtConfig, since plugins installed from imported system modules are inherently global. This does have some pitfalls when it comes to running tests, if a test context imports and thus also installs some plugin forward models to the ErtConfig, these will persist as classvars of the ErtConfig for subsequent test runs. So for example if we run

pm = ErtPluginManager(plugins=[dummy_plugins])
    run_cli_with_plugin_manager(
        [TEST_RUN_MODE, "--disable-monitor", str(tmp_path / "test.ert")], pm
    )

in one test, then the installed plugins will "show up" in subsequent tests, so this test would fail:

def test_that_include_take_into_account_path():
    ...
    assert list(ert_config.installed_forward_model_steps.keys()) == [
        "job1",
        "job2",
    ]

because the DummyForwardModel installed in the previous step would also show up. However when we run ert, we only create one config and don't care about creating multiple configs with separate sets of extensions within the same python context.

Issue
Resolves #7409

codecov-commenter · 2024-05-14T09:07:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.01%. Comparing base (36c89fb) to head (8295f62).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #7743       +/-   ##
===========================================
+ Coverage   68.61%   86.01%   +17.40%     
===========================================
  Files         381      382        +1     
  Lines       23482    23586      +104     
  Branches      633      629        -4     
===========================================
+ Hits        16112    20288     +4176     
+ Misses       7291     3225     -4066     
+ Partials       79       73        -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/unit_tests/config/test_forward_model.py

eivindjahren · 2024-05-31T10:41:45Z

src/ert/config/ert_config.py

+            try:
+                if not skip_pre_experiment_validation:
+                    fm_step_json = fm_step.validate_pre_realization_run(fm_step_json)
+            except BaseException as exc:


Instead of BaseException I think we should create a specific exception that forward model validation can raise to indicate invalid json. This is in order to avoid catching things like keyboard interrupt (Ctrl-C) or AttributeError (something is wrong with the plugin implementation):

class InvalidForwardModelCall(Exception): """ Thrown when the user calls the forward model incorrectly. Can be subtyped by the implementation of ForwardModelStepPlugin and thrown from `validate_pre_realization_run` or `validate_pre_experiment`. """

I added it to ert, called it ForwardModelInvalidStepCall, if you insist I will go with InvalidForwardModelCall but it is nice to be able to just type Fo.., in the IDE and get up all the forwardmodel stuff on autocomplete.

👍 You also need to just catch that exception when you call validate_pre_realization_run.

eivindjahren · 2024-05-31T10:43:28Z

src/ert/config/forward_model_step.py

+    on the queue.
+
+    Attributes:
+        name: A string representing the name of the job


name is obvious so maybe just omit or at least make the description shorter.

src/ert/config/forward_model_step.py

eivindjahren · 2024-05-31T10:53:48Z

src/ert/config/forward_model_step.py

+        self,
+        name: str,
+        command: List[str],
+        options: Optional[ForwardModelStepOptions] = None,


options here seems like a typical use case for https://typing.readthedocs.io/en/latest/spec/callables.html#unpack-kwargs . I suggest we put in **kwargs: Unpack[ForwardModelStepOptions] instead. If a plugin wants to use type the call to their ForwardModelStepPlugin we also need to expose ForwardModelStepOptions through ert/__init__.py.

Did not know about Unpack, so nice, updated semeio plugins accordingly equinor/semeio@055f28c

src/ert/shared/plugins/hook_specifications/forward_model_steps.py

eivindjahren · 2024-05-31T11:17:41Z

tests/unit_tests/config/test_forward_model.py

+
+            # It is in the command, but not in the forward model(...) entry in the
+            # ert config. Thus it is not a "private" arg in that sense.
+            # Note2reviewer: We COULD include references to variables in the


Maybe remove Note2reviewer and put that in github comments instead :D. I didn't understand exactly what you ment by this comment.

Ohh, I was trying to say that private_args will only include substituted args that are declared in the ert config, but not necessarily all the args in arglist.

So if arglist (in the fm step specification) is [x,y,z], but the forward model invocation in the ert config is this FORWARD_MODEL FM(<x>=2), then self.private_args will only contain <x>, and the concern was whether developers implementing a forward model would be interested in what y and z would be substituted to, even though it isn't specified in the forward model step invocation in the ert config.

So my question/concern was whether it was important to tweak the substitution logic to somehow make the substituted args from the entire arglist available, for validation purposes. If a developer implementing a fm step plugin says that X is in the arglist, and in the pre-experiment / pre-realization hook wants to know what it would be substituted with, regardless of whether it is invoked in the ert config fm step. I would assume this is never the case but am not completely sure.

We looked into this a while ago and we are pretty much unable to check the entire arglist because it seems that some user scripts handle unsubstituted arguments on their own.

Co-authored-by: Eivind Jahren <ejah@equinor.com>

…rward model steps

…ing to google style

eivindjahren · 2024-05-31T12:27:50Z

src/ert/config/forward_model_step.py

+    stderr_file: Optional[str]
+    start_file: Optional[str]
+    target_file: Optional[str]
+    error_file: Optional[str]


With TypedDict you need to use `NotRequired to indicate that the dictionary does not need to contain the keyword: https://typing.readthedocs.io/en/latest/spec/callables.html#required-and-non-required-keys .

eivindjahren · 2024-05-31T13:15:30Z

src/ert/config/__init__.py

@@ -52,6 +57,9 @@
    "ExternalErtScript",
    "Field",
    "ForwardModelStep",
+    "ForwardModelStepPlugin",
+    "ForwardModelStepJSON",
+    "ForwardModelInvalidCallError",


The -Error is a common postfix for Exceptions, so is -Exception. I would rather we either just called it InvalidForwardModelCall which is a more descriptive name without any postfix, or we go with ForwardModelValidationError which has the postfix and otherwise is better gramatically.

Yep, I think ForwardModelValidationError is nice and more in-tune with other naming we currently have in ert, and is easier to find/lookup when looking for stuff related to ForwardModel. Also personal preference but I think having Error postfix is nice, esp for just looking up stuff when developing. Just typing in something like fmer would find a fuzzy match on this, but on InvalidForwardModelCall it'd be a bit further away from forward model in a fuzzy-textual manner.

Also the errors we raise in the validation steps pre_experiment and pre_realization_run are per definition validation errors. What exactly they validate (state or input arguments to the forward model) is arbitrary, but they are still distinct from errors that arise during the run of the forward model.

I actually think it should be ForwardModelStepValidationError just to be explicit about the forward model vs forward model step, in spite of it being a bit more verbose

eivindjahren

👍

yngve-sk self-assigned this Apr 23, 2024

yngve-sk marked this pull request as draft April 23, 2024 13:00

yngve-sk force-pushed the plugin-forward-model branch 2 times, most recently from 278a961 to 49e1756 Compare April 26, 2024 13:22

yngve-sk force-pushed the plugin-forward-model branch 3 times, most recently from b3c863a to bb22569 Compare May 13, 2024 08:39

yngve-sk mentioned this pull request May 13, 2024

Trim jobs.json content #7876

Merged

yngve-sk force-pushed the plugin-forward-model branch 5 times, most recently from 9734629 to 5d85938 Compare May 14, 2024 08:46

yngve-sk marked this pull request as ready for review May 14, 2024 08:46

yngve-sk marked this pull request as draft May 14, 2024 09:47

yngve-sk force-pushed the plugin-forward-model branch 11 times, most recently from ef4d6e2 to f190186 Compare May 21, 2024 10:00

yngve-sk marked this pull request as ready for review May 21, 2024 10:23

yngve-sk mentioned this pull request May 23, 2024

Forward models as ert plugins inheriting from ForwardModelStep equinor/semeio#613

Merged

yngve-sk force-pushed the plugin-forward-model branch from 52bb845 to 219ab56 Compare May 23, 2024 10:08

eivindjahren reviewed May 30, 2024

View reviewed changes

tests/unit_tests/config/test_forward_model.py Outdated Show resolved Hide resolved

eivindjahren reviewed May 30, 2024

View reviewed changes

tests/unit_tests/config/test_forward_model.py Outdated Show resolved Hide resolved

eivindjahren reviewed May 30, 2024

View reviewed changes

tests/unit_tests/config/test_forward_model.py Outdated Show resolved Hide resolved

eivindjahren reviewed May 30, 2024

View reviewed changes

tests/unit_tests/config/test_forward_model.py Outdated Show resolved Hide resolved

eivindjahren reviewed May 31, 2024

View reviewed changes

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

eivindjahren reviewed May 31, 2024

View reviewed changes

src/ert/shared/plugins/hook_specifications/forward_model_steps.py Outdated Show resolved Hide resolved

eivindjahren reviewed May 31, 2024

View reviewed changes

src/ert/shared/plugins/hook_specifications/forward_model_steps.py Show resolved Hide resolved

eivindjahren reviewed May 31, 2024

View reviewed changes

Yngve S. Kristiansen and others added 6 commits May 31, 2024 14:00

Add possiblity to specify forward models as plugins

2d426bf

Add tests for using fm step plugins

7fb3aa5

Update tests/integration_tests/test_cli.py

d102dd7

Co-authored-by: Eivind Jahren <ejah@equinor.com>

Address review comments

fea3a7b

Address review comment: Add specific error to be raised for plugin fo…

6c26051

…rward model steps

Address review comment: Reformat fm step documentation comment accord…

0325114

…ing to google style

yngve-sk force-pushed the plugin-forward-model branch from a4e33d3 to 0325114 Compare May 31, 2024 12:04

Yngve S. Kristiansen added 3 commits May 31, 2024 14:05

Use unpack kwargs instead of explicit options

efd7516

Some small fixes

045d7dc

Remove note2reviewer segment from comment

80f84fc

yngve-sk requested a review from eivindjahren May 31, 2024 12:10

eivindjahren reviewed May 31, 2024

View reviewed changes

Yngve S. Kristiansen added 2 commits May 31, 2024 14:35

Address review: some final fixups

d7234a9

Use NotRequired

8295f62

yngve-sk requested a review from sondreso May 31, 2024 12:44

eivindjahren reviewed May 31, 2024

View reviewed changes

Rename error

e8e4074

eivindjahren approved these changes Jun 3, 2024

View reviewed changes

yngve-sk merged commit ceda74f into equinor:main Jun 3, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forward models as plugins #7743

Add forward models as plugins #7743

yngve-sk commented Apr 23, 2024 •

edited by eivindjahren

codecov-commenter commented May 14, 2024 •

edited

eivindjahren May 31, 2024 •

edited

yngve-sk May 31, 2024

eivindjahren May 31, 2024

eivindjahren May 31, 2024

yngve-sk May 31, 2024

eivindjahren May 31, 2024

yngve-sk May 31, 2024

eivindjahren May 31, 2024 •

edited

yngve-sk May 31, 2024

yngve-sk May 31, 2024

eivindjahren May 31, 2024

eivindjahren May 31, 2024

eivindjahren May 31, 2024

yngve-sk May 31, 2024

yngve-sk May 31, 2024

eivindjahren left a comment

Add forward models as plugins #7743

Add forward models as plugins #7743

Conversation

yngve-sk commented Apr 23, 2024 • edited by eivindjahren

codecov-commenter commented May 14, 2024 • edited

Codecov Report

eivindjahren May 31, 2024 • edited

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

eivindjahren May 31, 2024 • edited

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

eivindjahren left a comment

Choose a reason for hiding this comment

yngve-sk commented Apr 23, 2024 •

edited by eivindjahren

codecov-commenter commented May 14, 2024 •

edited

eivindjahren May 31, 2024 •

edited

eivindjahren May 31, 2024 •

edited