Add new forward model configuration style #10597

yngve-sk · 2025-04-10T10:06:15Z

Issue
Resolves #9615

codspeed-hq · 2025-04-10T10:29:01Z

CodSpeed Performance Report

Merging #10597 will not alter performance

_{Comparing yngve-sk:25.04.10.everest-fm-config-change (93aaabf) with main (527d97b)}

Summary

✅ 25 untouched benchmarks

oyvindeide

Looking good! Some comments which might simplify it a bit

src/everest/config/everest_config.py

oyvindeide · 2025-04-22T13:23:17Z

src/everest/config/everest_config.py

            if job_name not in installed_jobs_name:
                errors.append(f"unknown job {job_name}")

        if len(errors) > 0:  # Note: python3.11 ExceptionGroup will solve this nicely
            raise ValueError(errors)
        return self

+    def get_forward_model_steps(
+        self, result_type: Literal["gen_data", "summary"]
+    ) -> list[ForwardModelStepConfigWithResults]:


This will also be simplified if you add a field validator on forward_model I think

Removed this and moved filtering to the 2 callsites, behavior wrt field validator remains the same though.

src/everest/config/everest_config.py

oyvindeide · 2025-04-29T07:15:07Z

src/everest/config/everest_config.py

+        data_file = values.get("model", {}).get("data_file", None)
+        eclbase = values.get("definitions", {}).get("eclbase", None)


Use these values in the error message?

oyvindeide · 2025-04-29T07:16:53Z

src/everest/config/everest_config.py

+            message = "\n".join(
+                [
+                    "model.data_file is deprecated. ",
+                    "Instead, use install_data to specify the data file: ",


This was not done by data_file before? So this message is not needed, only the summary loading is affected.

So think we only need the message below. eclbase in definitions is still fine to do, but it will no longer have a special meaning. We can fix that by putting a note in the documentation.

oyvindeide · 2025-04-29T08:38:23Z

src/everest/config/everest_config.py

+        if self.forward_model is None:
+            return []


Dont think it can be None anymore

oyvindeide · 2025-04-29T08:40:54Z

src/everest/config/model_config.py

+    @model_validator(mode="before")
+    @classmethod
+    def validate_no_data_file(cls, values: dict[str, Any]) -> dict[str, Any]:
+        if "data_file" in values:


Double validation?

oyvindeide · 2025-04-29T08:43:05Z

src/everest/simulator/everest_to_ert.py

-    )
+    summary_fms = [
+        fm
+        for fm in (ever_config.forward_model or [])


Defaults to empty list now

oyvindeide · 2025-04-29T08:45:18Z

src/everest/simulator/everest_to_ert.py

+
+    requested_keys = summary_fm.results.keys
+
+    if requested_keys == "*":


This makes the default different from ert, where * means all the keys in the summary file.

Set it to requested keys + default ones for now. Should maybe revisit the behavior here later.

oyvindeide · 2025-04-29T08:45:46Z

src/everest/simulator/everest_to_ert.py

@@ -353,7 +345,7 @@ def _extract_forward_model(
 ) -> None:
    forward_model = _extract_data_operations(ever_config)
    forward_model += _extract_templating(ever_config)
-    forward_model += ever_config.forward_model or []
+    forward_model += ever_config.forward_model_step_commands or []


Defaults to empty list?

oyvindeide · 2025-04-29T08:46:43Z

src/everest/config/forward_model_config.py

+    type: Literal["gen_data", "summary"] = Field(
+        description="Type of the result, either 'gen_data' or 'summary'"
+    )
+    keys: Literal["*"] | list[str] = Field(


Only used in the case of summary?

Yep, special casing, could instead accept ["*"] and have only a list[str] type and have the same behavior?

ForwardModelStepResultsConfig could be:

ForwardModelStepResultsConfig = SummaryConfig | GenDataConfig

with a new field: type, then you can discriminate on type

oyvindeide · 2025-04-29T08:47:12Z

test-data/everest/egg/everest/model/config.yml

+  - job: eclipse100 eclipse/model/EGG.DATA --version 2020.2
+    results:
+      file_name: eclipse/model/EGG
+      type: summary
+  - rf -s eclipse/model/EGG -o rf


oyvindeide · 2025-04-29T11:53:59Z

src/everest/config/everest_config.py

+            if isinstance(fm, dict):
+                return fm
+
+            return {"job": fm, "results": None}


Beyond the scope, but could use this directly?

ert/src/ert/config/forward_model_step.py

Line 30 in 0793f26

class ForwardModelStepJSON(TypedDict):

. Do you have a feeling how much work that would be? Not for this PR, would be a separate issue

I guess we would have to make ERT also specify results along with forward models for it to be compatible like this, currently we have own keywords in the ERT config. Also the ERT config is less natural to give as a hierarchy than the everest config with yml. Would maybe defer to do as a future thing if we have the ERT config as purely yml/json. But a similar change in ERT would make sense conceptually.

oyvindeide · 2025-04-29T11:56:29Z

test-data/everest/egg/everest/model/config_flow.yml

@@ -1,5 +1,5 @@
 definitions:
-  eclbase: eclipse/model/EGG
+  realization_folder: r{{configpath}}/../../eclipse/include/realizations/realization-r{{realization}}/eclipse


Not needed? Can just remove the whole definitions section?

Actually might make sense to keep eclbase here as it is being reused later in the config.

oyvindeide · 2025-04-29T11:57:12Z

test-data/everest/egg/everest/model/config_flow.yml

@@ -121,8 +120,7 @@ simulator:
    max_running: 3

 install_data:
-  -
-    source: r{{ configpath }}/../../eclipse/include/realizations/realization-r{{ realization }}/eclipse
+  - source: r{{realization_folder}}


Related to comment above, can just keep the old? It is not being reused.

oyvindeide · 2025-04-29T11:58:30Z

test-data/everest/math_func/config_advanced.yml

+      file_name: distance_nonobj
+      type: gen_data
+  # Write the value of each control to a separate file
+  - job: adv_dump_controls --controls-file point.json --out-suffix _coord


Keep this without job: as it is not adding any results?

oyvindeide · 2025-04-29T12:01:02Z

tests/everest/functional/test_main_everest_entry.py

-@skipif_no_everest_models
-@pytest.mark.everest_models_test
-@pytest.mark.integration_test
-def test_everest_main_configdump_entry(copy_egg_test_data_to_tmp):


Why remove this? Think the assert can just be updated.

Address review

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 4 times, most recently from d436308 to c08e84f Compare April 14, 2025 13:21

yngve-sk marked this pull request as ready for review April 15, 2025 07:08

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 2 times, most recently from f4edd63 to 7ccd960 Compare April 16, 2025 07:47

yngve-sk closed this Apr 22, 2025

yngve-sk reopened this Apr 22, 2025

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 11 times, most recently from 2e08023 to 3d84617 Compare April 22, 2025 11:36

yngve-sk self-assigned this Apr 22, 2025

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch from 3943605 to bfae075 Compare April 22, 2025 12:21

oyvindeide reviewed Apr 22, 2025

View reviewed changes

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 2 times, most recently from cee48fb to 50275f0 Compare April 23, 2025 07:11

oyvindeide reviewed Apr 29, 2025

View reviewed changes

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch from a182c95 to 7ef2639 Compare April 29, 2025 10:39

oyvindeide reviewed Apr 29, 2025

View reviewed changes

yngve-sk added 4 commits April 29, 2025 13:56

Support per-forward-model-step results in config

b62c833

Make Everest store gen data results

39edb73

Adapt summary handling to new config

13921fc

Test that non-objective/constraint gendata results are stored

8cd6eb2

oyvindeide reviewed Apr 29, 2025

View reviewed changes

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 3 times, most recently from bddcbe1 to ce7ecee Compare April 30, 2025 06:38

yngve-sk added 4 commits April 30, 2025 09:00

fixup! Support per-forward-model-step results in config

562a45c

fixup! Make Everest store gen data results

09885ad

fixup! Adapt summary handling to new config

4af9ff4

Fix EverestConfig.with_defaults argument in test

93a0ef3

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch from ce7ecee to 9ac4bf0 Compare April 30, 2025 07:06

fixup! Support per-forward-model-step results in config

35e6c31

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch 2 times, most recently from 112e591 to 0af90ee Compare April 30, 2025 07:14

fixup! Adapt summary handling to new config

93aaabf

Address review

yngve-sk force-pushed the 25.04.10.everest-fm-config-change branch from 0af90ee to 93aaabf Compare April 30, 2025 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new forward model configuration style #10597

Add new forward model configuration style #10597

yngve-sk commented Apr 10, 2025

codspeed-hq bot commented Apr 10, 2025 •

edited

Loading

oyvindeide left a comment

oyvindeide Apr 22, 2025

yngve-sk Apr 23, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

yngve-sk Apr 30, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

yngve-sk Apr 29, 2025

oyvindeide Apr 29, 2025 •

edited

Loading

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

yngve-sk Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025

oyvindeide Apr 29, 2025 •

edited

Loading

yngve-sk Apr 30, 2025

		data_file = values.get("model", {}).get("data_file", None)
		eclbase = values.get("definitions", {}).get("eclbase", None)


		requested_keys = summary_fm.results.keys

		if requested_keys == "*":

Add new forward model configuration style #10597

Are you sure you want to change the base?

Add new forward model configuration style #10597

Conversation

yngve-sk commented Apr 10, 2025

codspeed-hq bot commented Apr 10, 2025 • edited Loading

Merging #10597 will not alter performance

Summary

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

oyvindeide Apr 29, 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

oyvindeide Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codspeed-hq bot commented Apr 10, 2025 •

edited

Loading

oyvindeide Apr 29, 2025 •

edited

Loading

oyvindeide Apr 29, 2025 •

edited

Loading