Use Experiment server in all run models when feature is enabled #3768

berland · 2022-08-16T10:15:33Z

Issue
Resolves #3413

Approach
runSimulations() in each run model has been ported to an async function alongside that uses the experiment server.

Pre review checklist

Added appropriate release note label
PR title captures the intent of the changes, and is fitting for release notes.
Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

xjules · 2022-08-16T10:20:22Z

You can check this one too: #3637 and close it afterwards.

codecov-commenter · 2022-08-17T09:03:07Z

Codecov Report

Merging #3768 (c096236) into main (cd3b47b) will decrease coverage by 0.00%.
The diff coverage is 62.20%.

@@            Coverage Diff             @@
##             main    #3768      +/-   ##
==========================================
- Coverage   63.73%   63.73%   -0.01%     
==========================================
  Files         587      587              
  Lines       43828    43977     +149     
  Branches     3786     3786              
==========================================
+ Hits        27935    28029      +94     
- Misses      14770    14826      +56     
+ Partials     1123     1122       -1

Impacted Files	Coverage Δ
src/ert/experiment_server/_server.py	`80.41% <0.00%> (ø)`
...rc/ert/shared/models/iterated_ensemble_smoother.py	`60.00% <14.51%> (-28.78%)`	⬇️
src/ert/shared/models/single_test_run.py	`94.73% <50.00%> (-5.27%)`	⬇️
src/ert/shared/main.py	`81.25% <66.66%> (+0.79%)`	⬆️
src/ert/shared/models/ensemble_experiment.py	`95.38% <75.00%> (-0.14%)`	⬇️
src/ert/shared/models/ensemble_smoother.py	`94.30% <92.00%> (-1.59%)`	⬇️
src/ert/shared/models/base_run_model.py	`89.64% <100.00%> (+0.08%)`	⬆️
...rc/ert/shared/models/multiple_data_assimilation.py	`96.44% <100.00%> (+1.10%)`	⬆️
src/ert/data/record/_transformation.py	`88.57% <0.00%> (-0.48%)`	⬇️
src/clib/lib/res_util/block_fs.cpp	`67.62% <0.00%> (+0.40%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

berland · 2022-08-17T10:17:32Z

src/ert/shared/models/ensemble_smoother.py

+
+        # Push ensemble, parameters, observations to new storage
+        ensemble_id = await loop.run_in_executor(
+            threadpool, self._post_ensemble_data, update_id


_post_ensemble_data calls self.setPhaseName() which should maybe invoke a call to the experiment server?

That will happen later when the experiment server supports it.

xjules · 2022-08-31T07:10:42Z

src/ert/shared/models/ensemble_experiment.py

            experiment_logger.debug("evaluating")
            await self._evaluate(run_context, evaluator_server_config)

-            num_successful_realizations = self._state_machine.successful_realizations(
+            num_successful_realizations = await self.successful_realizations(


Agree, makes more sense to use the defined member 👍

xjules · 2022-08-31T07:48:25Z

src/ert/shared/models/ensemble_smoother.py

+            prior_context.iteration,
+        )
+
+        self._checkMinimumActiveRealizations(prior_context)


Use loop.run_in_executor? Not sure where should be a threshold for saying this function is "atomic" does not need to be async.

Tracing through the code, this seems to only involve trivial getters and an integer comparison. My gut feeling is then not to
yield through await since there might be some overhead with our switching between sync and async context? (this is partly speculation)

You are right there actually! For reference: https://stackoverflow.com/questions/54813458/asyncio-performance-with-synchronous-code

xjules · 2022-08-31T08:41:49Z

src/ert/shared/models/ensemble_smoother.py

+
+        experiment_logger.debug("update complete")
+
+        await self.dispatch(


We might consider creating a helper class for dispatch, which automatically injects the correct source string based on ids specified and creates an uuid.

Good idea, implemented. Unsure of the name I chose, "_dispatch_ee"

xjules · 2022-09-05T10:07:31Z

src/ert/shared/models/ensemble_smoother.py

+                        "id": str(uuid.uuid1()),
+                    },
+                    {
+                        "error": str(e),


Wondering where this error field is actually used? This is essentially data argument in CloudEvent, but it's not really processed ...., or maybe I've missed it where; hence the question :)

Not sure, but I guess it does make sense to pass the error information on. The same data argument is used for hook names as far as I can see, maybe not picked up (yet!) either(?)

I see. I wonder mainly whether this kind of general "data" field should be present for Protobuf objects (which of-course it can't, but we can supplement some error fields at least) ...
Anyway, for this I understand you would like to keep regardless it's being consumed or not?

berland · 2022-09-07T13:04:28Z

todo: Write test function for _dispatch_ee

xjules · 2022-09-07T13:42:01Z

todo: Write test function for _dispatch_ee

Considering _dispatch_ee again.... I think the code looks much more compact. Additional, since it only affects the evaluator than I would keep it without the test function since this call can be considered "atomic"; ie. essentially one liner ...

lars-petter-hauge · 2022-09-07T13:48:08Z

Thanks for picking up on the continuation of #3438. With this implementation though we are adding a fair bit of duplicated code for the run function, from what I can see because:

we want it to be async
we want the function called from experiment_server to also send events.

Could we not use the samme pattern that we already implement for evaluate?

There we have both def evaluate and async def evaluate_async, in which they both call async def _evaluate_inner. The synchronous version has to call the async version with get_event_loop.run_until_complete.

I would really like us to avoid duplicating the runSimulations functionality, as it will be a real hassle to keep both up to date (we will likely fail..).

The async versions are modified duplicates of the existing sync run functions, and use the Experiment server feature. Co-authored-by: Håvard Berland <havb@equinor.com>

xjules · 2022-09-26T14:01:17Z

Thanks for picking up on the continuation of #3438. With this implementation though we are adding a fair bit of duplicated code for the run function, from what I can see because:

we want it to be async

we want the function called from experiment_server to also send events.

Could we not use the samme pattern that we already implement for evaluate?

There we have both def evaluate and async def evaluate_async, in which they both call async def _evaluate_inner. The synchronous version has to call the async version with get_event_loop.run_until_complete.

I would really like us to avoid duplicating the runSimulations functionality, as it will be a real hassle to keep both up to date (we will likely fail..).

As discussed on our standup, we will do the "merging" process on one by one basis. The corresponding issue for the task is here: #3948.

xjules

Nice and LGTM! I linked to the corresponding issue to this PR.

berland force-pushed the experiment_api branch 3 times, most recently from 656549d to f452e3c Compare August 17, 2022 08:36

berland commented Aug 17, 2022

View reviewed changes

berland linked an issue Aug 18, 2022 that may be closed by this pull request

Implement experiment interface in all run models #3413

Closed

berland mentioned this pull request Aug 18, 2022

Experiment server API for all RunModels #3637

Closed

3 tasks

berland changed the title ~~Complete experiment API adoption~~ WIP: Use Experiment server in all run models Aug 18, 2022

berland added enhancement experiment-server labels Aug 18, 2022

berland force-pushed the experiment_api branch from 78a29e8 to 73f18e4 Compare August 19, 2022 13:04

berland self-assigned this Aug 29, 2022

berland force-pushed the experiment_api branch from 73f18e4 to 817f6ca Compare August 29, 2022 11:53

berland marked this pull request as ready for review August 29, 2022 12:39

berland changed the title ~~WIP: Use Experiment server in all run models~~ Use Experiment server in all run models Aug 29, 2022

xjules reviewed Aug 31, 2022

View reviewed changes

xjules reviewed Sep 5, 2022

View reviewed changes

berland added the release-notes:new-feature Automatically categorise as new feature in release notes label Sep 6, 2022

berland changed the title ~~Use Experiment server in all run models~~ Use Experiment server in all run models when feature is enabled Sep 6, 2022

berland force-pushed the experiment_api branch 2 times, most recently from bc1c370 to c096236 Compare September 6, 2022 07:26

xjules mentioned this pull request Sep 6, 2022

Consume Protobuf events from dispatcher and pass them to the statemachine #3733

Merged

3 tasks

berland force-pushed the experiment_api branch from c096236 to 8e83349 Compare September 6, 2022 13:39

berland force-pushed the experiment_api branch from 8e83349 to 55190c8 Compare September 8, 2022 13:41

berland force-pushed the experiment_api branch 2 times, most recently from 5daa7d2 to ca511b1 Compare September 19, 2022 15:05

Add async run() functions for all run models

afe7d88

The async versions are modified duplicates of the existing sync run functions, and use the Experiment server feature. Co-authored-by: Håvard Berland <havb@equinor.com>

berland force-pushed the experiment_api branch from ca511b1 to afe7d88 Compare September 21, 2022 13:06

xjules mentioned this pull request Sep 26, 2022

Create a single runSimulations function to be used in both async and sync context for all run models #3948

Open

xjules approved these changes Sep 26, 2022

View reviewed changes

berland merged commit 96bfc31 into equinor:main Sep 27, 2022

berland deleted the experiment_api branch October 18, 2022 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Experiment server in all run models when feature is enabled #3768

Use Experiment server in all run models when feature is enabled #3768

berland commented Aug 16, 2022 •

edited

xjules commented Aug 16, 2022

codecov-commenter commented Aug 17, 2022 •

edited

berland Aug 17, 2022

berland Aug 29, 2022

xjules Aug 31, 2022 •

edited

xjules Aug 31, 2022 •

edited

berland Sep 6, 2022

xjules Sep 6, 2022

xjules Aug 31, 2022 •

edited

berland Sep 6, 2022

xjules Sep 5, 2022 •

edited

berland Sep 6, 2022

xjules Sep 6, 2022

berland commented Sep 7, 2022

xjules commented Sep 7, 2022

lars-petter-hauge commented Sep 7, 2022

xjules commented Sep 26, 2022 •

edited

xjules left a comment


		experiment_logger.debug("update complete")

		await self.dispatch(

Use Experiment server in all run models when feature is enabled #3768

Use Experiment server in all run models when feature is enabled #3768

Conversation

berland commented Aug 16, 2022 • edited

Pre review checklist

xjules commented Aug 16, 2022

codecov-commenter commented Aug 17, 2022 • edited

Codecov Report

berland Aug 17, 2022

Choose a reason for hiding this comment

berland Aug 29, 2022

Choose a reason for hiding this comment

xjules Aug 31, 2022 • edited

Choose a reason for hiding this comment

xjules Aug 31, 2022 • edited

Choose a reason for hiding this comment

berland Sep 6, 2022

Choose a reason for hiding this comment

xjules Sep 6, 2022

Choose a reason for hiding this comment

xjules Aug 31, 2022 • edited

Choose a reason for hiding this comment

berland Sep 6, 2022

Choose a reason for hiding this comment

xjules Sep 5, 2022 • edited

Choose a reason for hiding this comment

berland Sep 6, 2022

Choose a reason for hiding this comment

xjules Sep 6, 2022

Choose a reason for hiding this comment

berland commented Sep 7, 2022

xjules commented Sep 7, 2022

lars-petter-hauge commented Sep 7, 2022

xjules commented Sep 26, 2022 • edited

xjules left a comment

Choose a reason for hiding this comment

berland commented Aug 16, 2022 •

edited

codecov-commenter commented Aug 17, 2022 •

edited

xjules Aug 31, 2022 •

edited

xjules Aug 31, 2022 •

edited

xjules Aug 31, 2022 •

edited

xjules Sep 5, 2022 •

edited

xjules commented Sep 26, 2022 •

edited