Internalize ert_templates into storage #10617

xjules · 2025-04-11T13:21:57Z

Issue
This is a necessary pre-work related to #10180

All templates will be part of storage and thus for instance restart would
rely only on templates from the storage. Additionally, this removes templates
param from create_run_path, but it needs to be specified directly when calling
create_experiment.
Templates are stored in the experiment.mount_point / templates folder
Example: [["templates/seed_template_0.txt", "seed.txt"]]

It requires storage migration for run_templates.

(Screenshot of new behavior in GUI if applicable)

PR title captures the intent of the changes, and is fitting for release notes.
Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.
Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

When there are user facing changes: Updated documentation
New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
Large PR: Prepare changes in small commits for more convenient review
Bug fix: Add regression test for the bug
Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

codspeed-hq · 2025-04-11T13:44:01Z

CodSpeed Performance Report

Merging #10617 will not alter performance

_{Comparing xjules:storage_template (83152de) with main (2e9a852)}

Summary

✅ 25 untouched benchmarks

src/ert/storage/local_experiment.py

jonathan-eq · 2025-04-15T08:21:58Z

src/ert/storage/local_experiment.py

@@ -248,6 +265,19 @@ def parameter_info(self) -> dict[str, Any]:
            info = json.load(f)
        return info

+    @cached_property


Do we want this as cached_property? The others are just property

Not really, if you check parameter_configuration for instance. We don't want to access the files all the time.

Probaby dont want this as a cached property now as we are keeping file content in memory?

jonathan-eq

Some nitpicking 👍

src/ert/run_models/base_run_model.py

src/ert/storage/local_experiment.py

xjules · 2025-04-16T09:56:48Z

Semeio failure relates to something else, most likely SmootherUpdate API change.

oyvindeide

Looks good! Some minor comments, and one corner case

oyvindeide · 2025-04-22T12:21:10Z

src/ert/run_models/base_run_model.py

-from ..plugins.workflow_fixtures import (
-    create_workflow_fixtures_from_hooked,
-)
+from ..plugins.workflow_fixtures import create_workflow_fixtures_from_hooked


Dont mind this change, but should be in a different commit as it is not related to the main change?

~~Will create a new commit for these imports...~~
I have removed the changes to make the PR more compact.

oyvindeide · 2025-04-22T12:21:43Z

src/ert/run_models/everest_run_model.py

-    EverestStorage,
-    OptimalResult,
-)
+from everest.everest_storage import EverestStorage, OptimalResult


Same comment as above, fine to do, but should be its own commit

oyvindeide · 2025-04-22T12:21:58Z

src/ert/run_models/everest_run_model.py

-    EverestCacheHitEvent,
-    EverestStatusEvent,
-)
+from .event import EverestBatchResultEvent, EverestCacheHitEvent, EverestStatusEvent


Same as above

oyvindeide · 2025-04-22T12:22:17Z

src/ert/run_models/everest_run_model.py

+                    result_type=(
+                        "FunctionResult"
+                        if isinstance(r, FunctionResults)
+                        else "GradientResult"
+                    ),


Same comment

oyvindeide · 2025-04-22T12:22:29Z

src/ert/run_models/everest_run_model.py

+            perturbations=(
+                evaluator_context.perturbations.tolist()
+                if evaluator_context.perturbations is not None
+                else [-1] * len(model_realizations)
+            ),


Same comment

oyvindeide · 2025-04-22T12:22:38Z

src/ert/run_models/multiple_data_assimilation.py

-    ESSettings,
-    UpdateSettings,
-)
+from ert.config import ConfigValidationError, ErtConfig, ESSettings, UpdateSettings


Same comment

oyvindeide · 2025-04-22T12:38:38Z

src/ert/storage/local_experiment.py

+            for src, dst in templates:
+                incoming_template_file_path = Path(src)
+                template_file_path = Path(
+                    templates_path / incoming_template_file_path.name


This will fail in cases where I have different templates with the same name, for example snake oil:

RUN_TEMPLATE templates/seed_template.txt seed.txt RUN_TEMPLATE seed_template.txt seed_2.txt

valid input, but it will overwrite the template.

A bit of a corner case, but something to consider? Could hash the input file name for example

Should be fixed now. Each template is stored as relative path with an index enumerating the list:

[["templates/seed_template_0.txt", "seed.txt"]]

oyvindeide

LGTM! Some memory questions though

oyvindeide · 2025-04-23T13:45:06Z

src/ert/enkf_main.py

+            for (
+                source_file_content,
+                target_file,
+            ) in ensemble.experiment.templates_configuration:


We are loading all the template files at the same time now, but should be ok memory wise I guess?

It will be loaded on the fly now.

oyvindeide · 2025-04-23T13:45:45Z

src/ert/storage/local_experiment.py

@@ -248,6 +265,19 @@ def parameter_info(self) -> dict[str, Any]:
            info = json.load(f)
        return info

+    @cached_property


Probaby dont want this as a cached property now as we are keeping file content in memory?

All templates will be part of storage and thus for instance restart would rely only on templates from the storage. Additionally, this removes templates param from create_run_path, but it needs to be specified directly when calling create_experiment. Templates are stored in the experiment.mount_point / templates folder Example: [["templates/seed_template_0.txt", "seed.txt"]] It requires storage migration for run_templates.

xjules self-assigned this Apr 11, 2025

xjules added this to SCOUT Apr 11, 2025

xjules moved this to In Progress in SCOUT Apr 11, 2025

xjules marked this pull request as ready for review April 13, 2025 21:11

xjules changed the title ~~Internalize ert_tepmlates into storage~~ Internalize ert_templates into storage Apr 13, 2025

xjules added release-notes:misc Automatically categorise as miscellaneous change in release notes improvement Something nice to have, that will make life easier for developers or users or both. labels Apr 15, 2025

jonathan-eq reviewed Apr 15, 2025

View reviewed changes

src/ert/storage/local_experiment.py Outdated Show resolved Hide resolved

jonathan-eq reviewed Apr 15, 2025

View reviewed changes

src/ert/storage/local_experiment.py Outdated Show resolved Hide resolved

jonathan-eq reviewed Apr 15, 2025

View reviewed changes

src/ert/storage/local_experiment.py Outdated Show resolved Hide resolved

jonathan-eq reviewed Apr 15, 2025

View reviewed changes

oyvindeide reviewed Apr 15, 2025

View reviewed changes

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

src/ert/storage/local_experiment.py Outdated Show resolved Hide resolved

src/ert/storage/local_experiment.py Outdated Show resolved Hide resolved

xjules force-pushed the storage_template branch from d36987e to c5ffebf Compare April 15, 2025 12:00

xjules requested review from oyvindeide and jonathan-eq April 15, 2025 12:01

xjules moved this from In Progress to Ready for Review in SCOUT Apr 15, 2025

xjules force-pushed the storage_template branch from c8b8c3b to c1da5e1 Compare April 15, 2025 21:05

oyvindeide reviewed Apr 22, 2025

View reviewed changes

xjules force-pushed the storage_template branch 2 times, most recently from 6db164f to b714e1a Compare April 23, 2025 00:17

xjules requested a review from oyvindeide April 23, 2025 10:44

xjules force-pushed the storage_template branch 2 times, most recently from c0102f2 to 1723f64 Compare April 23, 2025 12:58

oyvindeide approved these changes Apr 23, 2025

View reviewed changes

github-project-automation bot moved this from Ready for Review to Reviewed in SCOUT Apr 23, 2025

xjules force-pushed the storage_template branch from 1723f64 to 83152de Compare April 23, 2025 14:14

xjules merged commit 469cae2 into equinor:main Apr 23, 2025
27 checks passed

github-project-automation bot moved this from Reviewed to Done in SCOUT Apr 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internalize ert_templates into storage #10617

Internalize ert_templates into storage #10617

xjules commented Apr 11, 2025 •

edited

Loading

codspeed-hq bot commented Apr 11, 2025 •

edited

Loading

jonathan-eq Apr 15, 2025

xjules Apr 15, 2025

oyvindeide Apr 23, 2025

jonathan-eq left a comment

xjules commented Apr 16, 2025 •

edited

Loading

oyvindeide left a comment

oyvindeide Apr 22, 2025

xjules Apr 22, 2025 •

edited

Loading

oyvindeide Apr 22, 2025

oyvindeide Apr 22, 2025

oyvindeide Apr 22, 2025

oyvindeide Apr 22, 2025

oyvindeide Apr 22, 2025

oyvindeide Apr 22, 2025

xjules Apr 23, 2025

oyvindeide left a comment

oyvindeide Apr 23, 2025

xjules Apr 23, 2025

oyvindeide Apr 23, 2025

Internalize ert_templates into storage #10617

Internalize ert_templates into storage #10617

Conversation

xjules commented Apr 11, 2025 • edited Loading

When applicable

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

Merging #10617 will not alter performance

Summary

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

xjules commented Apr 16, 2025 • edited Loading

oyvindeide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjules Apr 22, 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 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

xjules commented Apr 11, 2025 •

edited

Loading

codspeed-hq bot commented Apr 11, 2025 •

edited

Loading

xjules commented Apr 16, 2025 •

edited

Loading

xjules Apr 22, 2025 •

edited

Loading