Remove the possibility for multiple Steps in a Realization, remove Stage #6313

berland · 2023-10-12T13:56:59Z

Step "id" is removed, as there is now only one step (the "zero" step).

Issue
Partially resolves #6218

Approach
✂️ and removing the Step layer in several tree structures.

Pre review checklist

Read through the code changes carefully after finishing work
Make sure tests pass locally (after every commit!)
Prepare changes in small commits for more convenient review (optional)
PR title captures the intent of the changes, and is fitting for release notes.
Updated documentation
Ensured that unit tests are added for all new behavior (See
Ground Rules),
and changes to existing code have good test coverage.

Pre merge checklist

Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.

src/ert/ensemble_evaluator/_builder/_ensemble.py

src/ert/ensemble_evaluator/snapshot.py

xjules · 2023-10-23T13:16:56Z

src/ert/gui/model/snapshot.py

@@ -121,13 +119,13 @@ def prerender(
            metadata[SORTED_REALIZATION_IDS] = sorted(snapshot.reals.keys(), key=int)
            metadata[SORTED_JOB_IDS] = defaultdict(dict)


Here you can do:

metadata[SORTED_JOB_IDS] = defaultdict(lambda: defaultdict(list)) for idx, _ in job_states.items(): real_id, job_id = idx metadata[SORTED_JOB_IDS][real_id].append(job_id)

With some modifications, done. Nice.

xjules · 2023-10-23T13:29:35Z

src/ert/gui/model/snapshot.py

@@ -487,10 +461,12 @@ def index(self, row: int, column: int, parent: QModelIndex = None) -> QModelInde
        if parent is None:
            parent = QModelIndex()
        if not self.hasIndex(row, column, parent):
+            # we end here when parent is real_index, that is wrong!!!


you can remove these comments now :)

xjules · 2023-10-23T13:39:24Z

tests/unit_tests/ensemble_evaluator/ensemble_evaluator_utils.py

-    def _shouldFailJob(self, real, step, job):
-        return (real, 0, step, job) in self.fail_jobs
+    def _shouldFailJob(self, real, job):
+        return (real, 0, job) in self.fail_jobs


What is the second parameter 0 - can we remove it?

Thats probably a step that we should remove. Nice catch!

xjules · 2023-10-24T07:31:49Z

tests/unit_tests/ensemble_evaluator/ensemble_evaluator_utils.py

@@ -194,11 +184,11 @@ def evaluate(self, config):
    def start(self):
        self._eval_thread.start()

-    def _shouldFailJob(self, real, step, job):
-        return (real, 0, step, job) in self.fail_jobs
+    def _shouldFailJob(self, real, job):


This function is used only once, so I think it can be inlined instead.

this self.fail_jobs list is never updated so the if-sentence using this is probably never hit. This is potentially something unfinished in the test code, and fixing it or removing it should be its own PR.

xjules · 2023-10-24T07:33:27Z

tests/unit_tests/ensemble_evaluator/ensemble_evaluator_utils.py


-    def addFailJob(self, real, step, job):
-        self.fail_jobs.append((real, 0, step, job))
+    def addFailJob(self, real, job):


✂️ - not in use.

xjules · 2023-10-24T07:36:03Z

One thing that I would include is a small comment (a paragraph perhaps?) explaining what's the purpose of this Step singleton under the realization.

Step "id" is removed, as there is now only one step (the "zero" step). The step is kept for now as it contains needed information for how to run the realization. This information is later to be merged upwards to the realization itself.

berland · 2023-10-24T11:16:50Z

One thing that I would include is a small comment (a paragraph perhaps?) explaining what's the purpose of this Step singleton under the realization.

Added a sentence in the git commit log. Looking at the source I find it readable due to the dataclass structure.

xjules

Amazing job @berland ! 🚀

berland force-pushed the remove_multistep branch 2 times, most recently from eb193e1 to 3c41825 Compare October 16, 2023 07:41

berland mentioned this pull request Oct 17, 2023

Create an event driven update tab in the GUI #6358

Closed

berland marked this pull request as ready for review October 18, 2023 07:04

berland marked this pull request as draft October 18, 2023 07:04

xjules reviewed Oct 18, 2023

View reviewed changes

src/ert/ensemble_evaluator/_builder/_ensemble.py Show resolved Hide resolved

xjules reviewed Oct 18, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Outdated Show resolved Hide resolved

berland force-pushed the remove_multistep branch from c0855d5 to 4c726d5 Compare October 19, 2023 12:29

berland marked this pull request as ready for review October 19, 2023 12:32

berland self-assigned this Oct 19, 2023

berland added maintenance Not a bug now but could be one day, repaying technical debt release-notes:maintenance Automatically categorise as maintenance change in release notes labels Oct 19, 2023

berland mentioned this pull request Oct 19, 2023

Remove Stage, Step and Job from ensemble_evaluator._builder #6218

Closed

berland changed the title ~~Remove the possibility for multiple Steps in a Realization~~ Remove the possibility for multiple Steps in a Realization, remove Stage Oct 19, 2023

berland force-pushed the remove_multistep branch 5 times, most recently from 2c8ddb1 to 7e65644 Compare October 20, 2023 07:58

This comment was marked as resolved.

Sign in to view

berland force-pushed the remove_multistep branch 3 times, most recently from faf8b06 to c66be59 Compare October 20, 2023 10:27

xjules reviewed Oct 20, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Outdated Show resolved Hide resolved

xjules reviewed Oct 20, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Show resolved Hide resolved

xjules reviewed Oct 20, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Show resolved Hide resolved

xjules reviewed Oct 20, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Show resolved Hide resolved

berland force-pushed the remove_multistep branch from 729e2a8 to a63584b Compare October 20, 2023 14:47

xjules reviewed Oct 23, 2023

View reviewed changes

src/ert/ensemble_evaluator/snapshot.py Outdated Show resolved Hide resolved

xjules reviewed Oct 23, 2023

View reviewed changes

xjules reviewed Oct 24, 2023

View reviewed changes

berland added 4 commits October 24, 2023 13:14

Remove description of ert3 forward models in documentation

aa05f1d

Rename function in snapshot.py

f484311

Use defaultdict to shorten code

d787e8b

berland force-pushed the remove_multistep branch from b5ee1f8 to d787e8b Compare October 24, 2023 11:16

xjules approved these changes Oct 24, 2023

View reviewed changes

berland merged commit b100fb3 into equinor:main Oct 24, 2023
44 checks passed

berland deleted the remove_multistep branch November 8, 2023 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the possibility for multiple Steps in a Realization, remove Stage #6313

Remove the possibility for multiple Steps in a Realization, remove Stage #6313

berland commented Oct 12, 2023 •

edited

Loading

This comment was marked as resolved.

xjules Oct 23, 2023

berland Oct 23, 2023

xjules Oct 23, 2023

berland Oct 23, 2023

xjules Oct 23, 2023 •

edited

Loading

berland Oct 23, 2023

xjules Oct 24, 2023

berland Oct 24, 2023

xjules Oct 24, 2023

berland Oct 24, 2023

xjules commented Oct 24, 2023

berland commented Oct 24, 2023

xjules left a comment

		@@ -121,13 +119,13 @@ def prerender(
		metadata[SORTED_REALIZATION_IDS] = sorted(snapshot.reals.keys(), key=int)
		metadata[SORTED_JOB_IDS] = defaultdict(dict)

Remove the possibility for multiple Steps in a Realization, remove Stage #6313

Remove the possibility for multiple Steps in a Realization, remove Stage #6313

Conversation

berland commented Oct 12, 2023 • edited Loading

Pre review checklist

Pre merge checklist

This comment was marked as resolved.

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 Oct 23, 2023 • 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

xjules commented Oct 24, 2023

berland commented Oct 24, 2023

xjules left a comment

Choose a reason for hiding this comment

berland commented Oct 12, 2023 •

edited

Loading

xjules Oct 23, 2023 •

edited

Loading