Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce read-only info class derived from EvaluatorServerConfig #3045

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

Blunde1
Copy link
Contributor

@Blunde1 Blunde1 commented Mar 8, 2022

Issue
Resolves #3044

Approach
Utilize EvaluatorServerConfig and make sure references are okay

  • Create a new read-only information class derived from EvaluatorServerConfig.
  • Employ the new class (called EvaluatorConnectionInfo)

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

@Blunde1 Blunde1 force-pushed the evaluator-tracker-signature branch 2 times, most recently from 130d967 to 73ec17d Compare March 8, 2022 07:13
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #3045 (b1b96c1) into main (6b59105) will decrease coverage by 0.02%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
- Coverage   65.47%   65.45%   -0.03%     
==========================================
  Files         641      641              
  Lines       50631    50637       +6     
  Branches     4440     4440              
==========================================
- Hits        33151    33142       -9     
- Misses      15986    16003      +17     
+ Partials     1494     1492       -2     
Impacted Files Coverage Δ
ert_gui/simulation/run_dialog.py 77.03% <ø> (+0.70%) ⬆️
ert_shared/status/tracker/factory.py 100.00% <ø> (ø)
ert_shared/status/tracker/evaluator.py 74.31% <75.00%> (-0.91%) ⬇️
ert_shared/ensemble_evaluator/monitor.py 91.52% <83.33%> (-0.42%) ⬇️
ert_shared/ensemble_evaluator/config.py 94.00% <94.44%> (-1.41%) ⬇️
ert_shared/cli/main.py 89.58% <100.00%> (ø)
ert_shared/ensemble_evaluator/ensemble/prefect.py 91.76% <100.00%> (+1.17%) ⬆️
ert_shared/ensemble_evaluator/evaluator.py 95.97% <100.00%> (ø)
ert_shared/ensemble_evaluator/narratives/proxy.py 66.17% <0.00%> (-27.95%) ⬇️
... 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 6b59105...b1b96c1. Read the comment docs.

@Blunde1 Blunde1 mentioned this pull request Mar 8, 2022
1 task
@@ -215,11 +204,11 @@ def request_termination(self):
return

with create_ee_monitor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good approach! Can this create function take directly ee_config as parameter too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, implemented it.
The same is likely true for SyncWebsocketDuplexer. Looking into this now.

@sondreso
Copy link
Collaborator

sondreso commented Mar 8, 2022

The EvaluatorServerConfig has much more functionality that consumers besides the server itself should not use. I don't think it's the best option to spread this config around more in the code than it already is. If we want something that can encapsulate this info that is always passed around together, I think we should create a new class, that is only the data. I think there has already been an attempt at this with the EvaluatorServerConfigInfo that is in the same file as the EvaluatorServerConfig, but it is maybe to specifically made for the dispatchers and not the clients. So if we are to change this, I would suggest creating a class with only the necessary data (read-only), that can serve the need for connection info for both the dispatchers and the clients. (Maybe a proper name could be EvaluatorConnectionInfo ?)

@Blunde1
Copy link
Contributor Author

Blunde1 commented Mar 8, 2022

@sondreso like this: cafea9a
Then use get_connection_info() from the ee_config where ee_config is now passed?

@xjules
Copy link
Contributor

xjules commented Mar 8, 2022

The EvaluatorServerConfig has much more functionality that consumers besides the server itself should not use. I don't think it's the best option to spread this config around more in the code than it already is. If we want something that can encapsulate this info that is always passed around together, I think we should create a new class, that is only the data. I think there has already been an attempt at this with the EvaluatorServerConfigInfo that is in the same file as the EvaluatorServerConfig, but it is maybe to specifically made for the dispatchers and not the clients. So if we are to change this, I would suggest creating a class with only the necessary data (read-only), that can serve the need for connection info for both the dispatchers and the clients. (Maybe a proper name could be EvaluatorConnectionInfo ?)

I've seen the EvaluatorServerConfigInfo class, which is made for dispatchers as you said. If we are not exploit the info class then what about to use a simple dict_cfg as we discussed yesterday @Blunde1 @sondreso ?

@Blunde1
Copy link
Contributor Author

Blunde1 commented Mar 8, 2022

The EvaluatorServerConfig has much more functionality that consumers besides the server itself should not use. I don't think it's the best option to spread this config around more in the code than it already is. If we want something that can encapsulate this info that is always passed around together, I think we should create a new class, that is only the data. I think there has already been an attempt at this with the EvaluatorServerConfigInfo that is in the same file as the EvaluatorServerConfig, but it is maybe to specifically made for the dispatchers and not the clients. So if we are to change this, I would suggest creating a class with only the necessary data (read-only), that can serve the need for connection info for both the dispatchers and the clients. (Maybe a proper name could be EvaluatorConnectionInfo ?)

I've seen the EvaluatorServerConfigInfo class, which is made for dispatchers as you said. If we are not exploit the info class then what about to use a simple dict_cfg as we discussed yesterday @Blunde1 @sondreso ?

I think passing a read-only EvaluatorConnectionInfo makes sense.
It seems the most on-point to what we want, not more nor less, and also would require very little refactoring

port,
token=None,
cert=None,
ee_config: EvaluatorServerConfig,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EvaluatorConnectionInfo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Will get to refactor everything now. But this is exactly it. : ) 👍

@Blunde1 Blunde1 changed the title Refactor EvaluatorTracker to use EvaluatorServerConfig Introduce read-only info class derived from EvaluatorServerConfig Mar 8, 2022
@Blunde1 Blunde1 force-pushed the evaluator-tracker-signature branch from 2271ce4 to ce4b968 Compare March 8, 2022 12:38
@Blunde1 Blunde1 force-pushed the evaluator-tracker-signature branch from ce4b968 to b1b96c1 Compare March 8, 2022 13:02
@Blunde1 Blunde1 enabled auto-merge (rebase) March 8, 2022 13:03
Copy link
Contributor

@xjules xjules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job Berent! Once tests are green 🚀

@Blunde1 Blunde1 merged commit de1e413 into equinor:main Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EvaluatorTracker should utilize EvaluatorServerConfig
4 participants