Cli monitor for ert3 #2960

xjules · 2022-02-22T13:39:34Z

Issue
Resolves #2944
Resolves #2894

Approach
This combines the following two PRs (which will be closed):
#2944
#2903

Pre review checklist

Added appropriate labels

Adding labels helps the maintainers when writing release notes, see sections and the
corresponding labels here: https://github.com/equinor/ert/blob/main/.github/release.yml

codecov-commenter · 2022-02-22T14:05:40Z

Codecov Report

Merging #2960 (9aa2f8f) into main (4475cce) will increase coverage by 0.03%.
The diff coverage is 89.07%.

❗ Current head 9aa2f8f differs from pull request most recent head 60ba2ec. Consider uploading reports for the commit 60ba2ec to get more accurate results

@@            Coverage Diff             @@
##             main    #2960      +/-   ##
==========================================
+ Coverage   65.43%   65.47%   +0.03%     
==========================================
  Files         640      641       +1     
  Lines       50599    50640      +41     
  Branches     4438     4438              
==========================================
+ Hits        33111    33158      +47     
+ Misses      15996    15986      -10     
- Partials     1492     1496       +4

Impacted Files	Coverage Δ
ert_shared/status/utils.py	`76.66% <ø> (-5.95%)`	⬇️
ert_shared/ensemble_evaluator/monitor.py	`90.00% <80.00%> (-1.53%)`	⬇️
ert/tracker/evaluator_tracker.py	`77.20% <84.78%> (ø)`
ert3/evaluator/_evaluator.py	`92.30% <91.22%> (-4.67%)`	⬇️
ert/tracker/__init__.py	`100.00% <100.00%> (ø)`
ert_data/loader.py	`85.81% <100.00%> (ø)`
ert_gui/simulation/tracker_worker.py	`54.76% <100.00%> (+1.27%)`	⬆️
ert_shared/ensemble_evaluator/config.py	`95.18% <100.00%> (+1.18%)`	⬆️
ert_shared/ensemble_evaluator/entity/snapshot.py	`94.79% <100.00%> (ø)`
ert_shared/status/tracker/factory.py	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4475cce...60ba2ec. Read the comment docs.

ert/tracker/evaluator_tracker.py

jondequinor

Sorry if this is still WIP, but I left some comments. Seems some stuff is missing.

Other than that, looks good! I like the concurrent.future use!

ert3/evaluator/_evaluator.py

jondequinor · 2022-02-22T18:16:20Z

ert3/evaluator/_evaluator.py


    return result


 def evaluate(
-    ensemble: Ensemble,
+    ensemble: Ensemble, cli: bool = True


What is the cli flag? Is it ever used?

It is used, but it might be set as default and instead having a gui there?

I'm not sure what you mean?

If you check line 115, EvaluatorTracker is not created by default but only when cli==True. It could be created by default, but do something special when we will go after gui.

ert3/evaluator/_evaluator.py

Blunde1 · 2022-03-01T12:30:02Z

Remember to delete _calculate_progress() and tracker_progress() from ert_shared.status.utils.py

This is okay since the legacy tracker is nuked.
We should see if there are other relevant functions in utils that should be defined in evaluator_tracker as well

Blunde1 · 2022-03-03T08:47:27Z

This is okay since the legacy tracker is nuked. We should see if there are other relevant functions in utils that should be defined in evaluator_tracker as well

Do this in a separate PR: #3006

Blunde1 · 2022-03-04T13:14:49Z

testERT3RunModel together with EvaluatorTracker

Blunde1 · 2022-03-07T12:23:26Z

~~Create argument **config_args for EvaluatorTracker(), removing multiple single arguments and created arguments in __init__~~
~~Assert that required arguments are actually passed~~
~~Construct arguments like url on the fly~~
Create issue for refactoring according to pydantic model when the PR is merged

Edit Did #3045 instead

ert_shared/status/utils.py

Blunde1 · 2022-03-07T17:33:57Z

#dd18455 introduces **config_args, which should really be a named argument of type EvaluatorServerConfig - we know exactly what is required (host and port) and what is optional (token and cert). There is no need for wildcard notation which introduces unnecessary flexibility.

Can we write an issue now that states "EvaluatorTracker should utilize EvaluatorServerConfig" (no mention to **config_args, resolve it and rebase onto it when it is merged? Then we avoid introducing **config_args to main,

It could of course be introduced directly in this PR, but in the interest of keeping the PR small, the above suggestion might be preferable?

Blunde1 · 2022-03-08T07:38:17Z

Can we write an issue now that states "EvaluatorTracker should utilize EvaluatorServerConfig" (no mention to **config_args, resolve it and rebase onto it when it is merged? Then we avoid introducing **config_args to main,

Refactoring EvaluatorTracker to use EvaluatorServerConfig in this PR: #3045

ert/tracker/evaluator_tracker.py

jondequinor · 2022-03-11T15:26:49Z

tests/ert_tests/status/test_utils.py

@@ -28,52 +27,3 @@ def test_format_running_time(self):

        for t in tests:
            self.assertEqual(t["expected"], format_running_time(t["seconds"]))
-
-    def test_calculate_progress(self):


Are these not possible to test?

Since _calculate_progress function was removed I guess there is no need of having this test anymore 🤷

Will rewrite this for the _progress() method of EvaluatorTracker where all the functionality now lies

Thanks. For reference https://github.com/equinor/ert/pull/2960/files#diff-0735d41fdd807327ac2ea6450664124e2b521281a613966c2254a8d33b887b25R194

Found a bug for calculation of _progress() #3092

I suggest we resolve this outside of this PR.
This was found because a test now needs to ensure that total realizations are also fetched correctly.
This was not the case before, as only the maths given correct numbers were tested.

jondequinor

Some minor comments, but this is really great 👏

tests/ert_tests/status/test_evaluator_tracker.py

ert_shared/ensemble_evaluator/entity/snapshot.py

ert/tracker/evaluator_tracker.py

ert3/evaluator/_evaluator.py

sondreso · 2022-03-14T14:27:20Z

Looks very good, only had some minor comments!

Blunde1 · 2022-03-14T20:48:45Z

@sondreso I separated the EvaluatorConnectionInfo into its own file, however it is still inside the tracker module under ert/tracker. This feels better but I still slightly misplaced. My feeling is that we should have something along the lines of

ert
- ensemble_evaluator
- - tracker
- - - evaluator_tracker.py
- - evaluator_connection_info.py (eventually included in config.py if this is moved from ert_shared)

what do you think?

sondreso · 2022-03-15T07:02:31Z

My feeling is that we should have something along the lines of
ert
- ensemble_evaluator
- - tracker
- - - evaluator_tracker.py
- - evaluator_connection_info.py (eventually included in config.py if this is moved from ert_shared)
what do you think?

I think that makes the most sense yes!

jondequinor

LGTM! THanks!

berland · 2022-03-16T09:05:48Z

tests/ert_tests/status/test_evaluator_tracker.py

@@ -48,9 +49,10 @@ def _track():

 @pytest.mark.timeout(60)
 @pytest.mark.parametrize(
-    "monitor_events,brm_mutations,expected_progress",
+    "run_model, monitor_events,brm_mutations,expected_progress",


perhaps "blacken" this string by adding space after comma manually.

berland · 2022-03-16T09:10:35Z

Tested on a couple of ert3 examples, works fine, also with --local-test-run. Increases the user experience!

It includes a new EvaluatorTracker with the mypy annotation located in ert.tracker.evaluator_tracker. ert3 run evaluation supports now the monitoring, which is enabled by default. A new ERT3RunModel is introduced, which is a simplified version of BaseRunModel. ERT3RunModel is required by legacy monitoring classes. A new EvaluatorConnectionInfo class is introduced used to exchange connection info between EvaluatorTracker and monitors. Co-authored-by: kvashchuka <kvashchuk.anna@gmail.com> Co-authored-by: blunde1 <BERL@equinor.com>

xjules assigned xjules and Blunde1 Feb 22, 2022

Blunde1 reviewed Feb 22, 2022

View reviewed changes

ert/tracker/evaluator_tracker.py Outdated Show resolved Hide resolved

jondequinor reviewed Feb 22, 2022

View reviewed changes

This was referenced Feb 25, 2022

CLI monitoring for ert3 #2944

Closed

Move and rename evaluator tracker to ert/ert #2903

Closed

sondreso mentioned this pull request Feb 28, 2022

Move cli monitoring from ert/ert_shared into ert/ert #2894

Closed

xjules force-pushed the cli-monitor-type-hints_jp branch from 1eed498 to 1d77338 Compare March 7, 2022 13:29

xjules commented Mar 7, 2022

View reviewed changes

ert_shared/status/utils.py Show resolved Hide resolved

xjules force-pushed the cli-monitor-type-hints_jp branch 2 times, most recently from 9cf822c to 2b28f67 Compare March 9, 2022 12:37

jondequinor reviewed Mar 9, 2022

View reviewed changes

ert/tracker/evaluator_tracker.py Outdated Show resolved Hide resolved

xjules force-pushed the cli-monitor-type-hints_jp branch 2 times, most recently from 07eacc4 to 21b6631 Compare March 11, 2022 14:23

jondequinor reviewed Mar 11, 2022

View reviewed changes

ert/tracker/evaluator_tracker.py Outdated Show resolved Hide resolved

jondequinor reviewed Mar 11, 2022

View reviewed changes

sondreso reviewed Mar 14, 2022

View reviewed changes

xjules force-pushed the cli-monitor-type-hints_jp branch from bf91dce to 814c38d Compare March 15, 2022 14:44

jondequinor approved these changes Mar 16, 2022

View reviewed changes

berland reviewed Mar 16, 2022

View reviewed changes

kvashchuka force-pushed the cli-monitor-type-hints_jp branch from 814c38d to cf0a130 Compare March 16, 2022 09:27

xjules merged commit f7d4a5b into equinor:main Mar 16, 2022

xjules deleted the cli-monitor-type-hints_jp branch March 16, 2022 10:54

berland added the ert3 label May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli monitor for ert3 #2960

Cli monitor for ert3 #2960

xjules commented Feb 22, 2022

codecov-commenter commented Feb 22, 2022 •

edited

jondequinor left a comment

jondequinor Feb 22, 2022

xjules Feb 24, 2022

jondequinor Feb 24, 2022

xjules Feb 24, 2022

Blunde1 commented Mar 1, 2022 •

edited

Blunde1 commented Mar 3, 2022

Blunde1 commented Mar 4, 2022 •

edited by xjules

Blunde1 commented Mar 7, 2022 •

edited

Blunde1 commented Mar 7, 2022

Blunde1 commented Mar 8, 2022

jondequinor Mar 11, 2022

xjules Mar 13, 2022 •

edited

Blunde1 Mar 14, 2022

jondequinor Mar 14, 2022

Blunde1 Mar 15, 2022

jondequinor left a comment

sondreso commented Mar 14, 2022

Blunde1 commented Mar 14, 2022

sondreso commented Mar 15, 2022

jondequinor left a comment

berland Mar 16, 2022

berland commented Mar 16, 2022

Cli monitor for ert3 #2960

Cli monitor for ert3 #2960

Conversation

xjules commented Feb 22, 2022

Pre review checklist

codecov-commenter commented Feb 22, 2022 • edited

Codecov Report

jondequinor 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

Blunde1 commented Mar 1, 2022 • edited

Blunde1 commented Mar 3, 2022

Blunde1 commented Mar 4, 2022 • edited by xjules

Blunde1 commented Mar 7, 2022 • edited

Blunde1 commented Mar 7, 2022

Blunde1 commented Mar 8, 2022

Choose a reason for hiding this comment

xjules Mar 13, 2022 • edited

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

jondequinor left a comment

Choose a reason for hiding this comment

sondreso commented Mar 14, 2022

Blunde1 commented Mar 14, 2022

sondreso commented Mar 15, 2022

jondequinor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berland commented Mar 16, 2022

codecov-commenter commented Feb 22, 2022 •

edited

Blunde1 commented Mar 1, 2022 •

edited

Blunde1 commented Mar 4, 2022 •

edited by xjules

Blunde1 commented Mar 7, 2022 •

edited

xjules Mar 13, 2022 •

edited