Store ert<->everest realization mapping #9767

yngve-sk · 2025-01-16T08:28:51Z

Note: Should not be merged until a consistent realization mapping is received from ROPT, replacing the logic in the second commit of this PR.

Issue
Resolves #9751
Resolves #9674

Approach
Short description of the approach

(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 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

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: Create Backport PR to latest release

codspeed-hq · 2025-01-16T08:40:37Z

CodSpeed Performance Report

Merging #9767 will not alter performance

_{Comparing yngve-sk:25.01.16.add-everest-realization-info-to-ert (af05d6a) with main (cc15585)}

Summary

✅ 25 untouched benchmarks

src/ert/run_models/everest_run_model.py

verveerpj

Some comments but basically solid.

verveerpj · 2025-02-07T11:34:04Z

src/ert/run_models/everest_run_model.py

+
+        realization_mapping: dict[int, EverestRealizationInfo] = {
+            idx: EverestRealizationInfo(
+                geo_realization=self._everest_config.model.realizations[real],


Can we internally already move from geo to model, I think we should encourage that in the long run.

verveerpj · 2025-02-07T11:59:59Z

src/ert/storage/everest_ensemble.py

+class EverestRealizationInfo(TypedDict):
+    geo_realization: int
+    perturbation: int | None  # None means it stems from unperturbed controls
+    # Q: Maybe we also need result ID, or no? Ref if we have multiple evaluations


Yes, we will probably, let's not worry about it now.

verveerpj · 2025-02-07T12:01:57Z

src/ert/storage/everest_experiment.py

+        for e in self.ensembles:
+            ens_parameters = {
+                group: e.ert_ensemble.load_parameters(group)
+                .to_dataarray()
+                .data.reshape((e.ert_ensemble.ensemble_size, -1))
+                for group in parameter_values
+            }


you think may need some caching here, or is there already in the storage?

This just points to parameters saved in xr files in ERT storage

verveerpj · 2025-02-07T12:22:20Z

src/ert/run_models/everest_run_model.py

+                    else None
+                ),
+            )
+            for idx, real in enumerate(evaluator_context.realizations)


This maps the indices of the control vectors to realizations/perturbations. Is that the intention? Because if the intention is to map simulation ID (= ert realization?) then this will fail if some control vectors are not evaluated due to being cached or being marked as inactive. You would need to do the same as I did in my recent PR to get the simulation ID's for the simulations that actually did run.

verveerpj · 2025-02-07T12:41:38Z

src/ert/run_models/everest_run_model.py

+                )["values"]
+
+                cached_data = (
+                    objectives.to_numpy() * -1,


Be careful after rebasing, the -1 is probably not needed anymore.

removed the -1

verveerpj · 2025-02-07T12:43:55Z

src/ert/run_models/everest_run_model.py

-                if constraints is not None:
-                    assert cached_constraints is not None
-                    constraints[control_idx, ...] = cached_constraints
+        for control_idx, (


I think caching is now always assumed to be on? We do have a flag for in in the configuration, maybe we should kill it.

oyvindeide

Some questions and comments

oyvindeide · 2025-02-09T20:40:05Z

src/ert/run_models/everest_run_model.py

-        )
+
+        # Keep for re-runs, will work in-memory
+        # 2DO: If an experiment for this exact config already exists,


Issue for this?

oyvindeide · 2025-02-09T20:50:12Z

src/ert/run_models/everest_run_model.py

-        return cached_results
+            }
+
+        cached_results2: dict[int, Any] = {}


Variables with numbers in them gets me a bit confused 😅 Could this be rewritten/simplified?

oops, this was a temporary dev artifact, removed

oyvindeide · 2025-02-09T20:52:21Z

src/ert/storage/everest_ensemble.py

+    ) -> None:
+        self._index.ert2ev_realization_mapping = realization_mapping
+        self._ert_ensemble._storage._write_transaction(
+            self.ert_ensemble._path / "everest_index.json",


Is this needed? Could we integrate this more closely into the regular Ensemble? No reason that could not have metadata

oyvindeide · 2025-02-09T20:53:01Z

src/ert/storage/local_storage.py

@@ -589,6 +590,15 @@ def _to_parquet_transaction(
            os.chmod(f.name, 0o660)
            os.rename(f.name, filename)

+    def create_everest_experiment(


Would it be possible to integrate this into a regular experiment?

It can be put there yes... but whether we should have the logic merged into the ert ensemble vs keep the everest logic in one place, should be discussed I think.

And this layer still writes its metadata files into the "normal" ert experiment wrt files etc, it is just one place where we can put all the everest-related storage logic. In practice it should be the same effect as if the code was put directly into the ert experiment.

There are some edge cases and possible cases where we might need to add in more everest-specific storage logic, will likely grow as we resolve issues:
#9937
#9938 ,

Wrt caching there is a case, where we might want one realization to be a copy/symlink of another (from a previous ensemble)

Also, the realization numbers per ensemble (in ert) only go per "active" evaluation, so if we want to evaluate realizations 0-15, but the previous example is a cache hit for realizations 0-5, we'll (currently) have a new ensemble with realizations 0-9, which is really [6-15]

I would say the more everest-specific logic we get in there, the more argument against putting everest-specific methods etc into the ERT LocalEnsemble, but again up for discussion.

yngve-sk self-assigned this Jan 16, 2025

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch 8 times, most recently from 4e70ffd to be7341e Compare January 27, 2025 08:20

verveerpj reviewed Jan 29, 2025

View reviewed changes

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

yngve-sk mentioned this pull request Jan 29, 2025

Add dev documentation on everest vs ert data models #9820

Merged

9 tasks

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch 4 times, most recently from d4d8a92 to 2202375 Compare January 29, 2025 12:54

This was referenced Feb 3, 2025

Everest Storage: Load parameters by pointing to ERT storage #9936

Open

Everest Storage: Load objectives|constraints as responses from ERT storage #9937

Open

Everest Storage: Load batch_ aggregations from ERT storage #9938

Open

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch from 2202375 to 3990831 Compare February 3, 2025 07:55

verveerpj reviewed Feb 7, 2025

View reviewed changes

oyvindeide reviewed Feb 9, 2025

View reviewed changes

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch from 3990831 to f892664 Compare February 11, 2025 14:02

Add classes for everest-specific storage logic

66badcc

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch from f892664 to 39226fe Compare February 12, 2025 11:50

yngve-sk added 2 commits February 12, 2025 12:53

Save everest realization metadata

29656c7

Use ERT storage for simulator cache

af05d6a

yngve-sk force-pushed the 25.01.16.add-everest-realization-info-to-ert branch from 39226fe to af05d6a Compare February 12, 2025 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store ert<->everest realization mapping #9767

Store ert<->everest realization mapping #9767

yngve-sk commented Jan 16, 2025 •

edited

Loading

codspeed-hq bot commented Jan 16, 2025 •

edited

Loading

verveerpj left a comment

verveerpj Feb 7, 2025

verveerpj Feb 7, 2025

verveerpj Feb 7, 2025

yngve-sk Feb 7, 2025

verveerpj Feb 7, 2025

verveerpj Feb 7, 2025

yngve-sk Feb 11, 2025

verveerpj Feb 7, 2025

oyvindeide left a comment

oyvindeide Feb 9, 2025

yngve-sk Feb 11, 2025

oyvindeide Feb 9, 2025

yngve-sk Feb 11, 2025 •

edited

Loading

oyvindeide Feb 9, 2025

oyvindeide Feb 9, 2025

yngve-sk Feb 10, 2025 •

edited

Loading

Store ert<->everest realization mapping #9767

Are you sure you want to change the base?

Store ert<->everest realization mapping #9767

Conversation

yngve-sk commented Jan 16, 2025 • edited Loading

When applicable

codspeed-hq bot commented Jan 16, 2025 • edited Loading

Merging #9767 will not alter performance

Summary

verveerpj 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

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

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

yngve-sk Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

yngve-sk commented Jan 16, 2025 •

edited

Loading

codspeed-hq bot commented Jan 16, 2025 •

edited

Loading

yngve-sk Feb 11, 2025 •

edited

Loading

yngve-sk Feb 10, 2025 •

edited

Loading