-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove ert config from batch sim #9070
Remove ert config from batch sim #9070
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9070 +/- ##
==========================================
+ Coverage 90.72% 90.75% +0.03%
==========================================
Files 350 350
Lines 21800 21833 +33
==========================================
+ Hits 19777 19814 +37
+ Misses 2023 2019 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4d3c21d
to
e0a7721
Compare
@@ -20,7 +20,7 @@ def execute_workflow( | |||
msg = "Workflow {} is not in the list of available workflows" | |||
logger.error(msg.format(workflow_name)) | |||
return | |||
runner = WorkflowRunner(workflow, storage, ert_config=ert_config) | |||
runner = WorkflowRunner(workflow=workflow, storage=storage, ert_config=ert_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK change, but purely cosmetic and unrelated to this PR? Could be in a separate commit
@@ -83,6 +83,164 @@ def site_config_location() -> str: | |||
) | |||
|
|||
|
|||
def create_forward_model_json( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supposing nothing was changed here, just a move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HMM, I see the number of parameters passed from the ert config became bit large, a lot of it is from having to make create_run_path
generic so it can be invoked from both places. And we do need to get the info from the ErtConfig somehow but the way it is currently structured is making this very verbose. It might be worth thinking a bit more about whether we need to use the same create_run_path
from everest as from ERT, I'm not entirely sure..
I think this deserves some more discussion at least. Would it maybe be possible to make BatchSimulator depend more directly on things from the EverestConfig instead sub-configs of the ErtConfig?
I agree that some functions have become very verbose. In particular BatchSimulator and BatchContext. Perhaps its worth having a discussion how we can improve those two. |
e0a7721
to
88fa3ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
88fa3ec
to
6f3d835
Compare
Issue
Resolves #9002
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"'
)When applicable