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.

Sign up for GitHub

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

Jump to bottom

Replace interface towards eclrun and flow using fmstep_config #9108

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Replace interface towards eclrun and flow using fmstep_config #9108

merged 1 commit into from
Jan 9, 2025

Conversation

berland
Copy link
Contributor

@berland berland commented Oct 31, 2024 •
edited by eivindjahren
Loading

Issue
Resolves #8925

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@berland berland marked this pull request as draft October 31, 2024 08:20
@berland berland self-assigned this Oct 31, 2024
@berland berland mentioned this pull request Oct 31, 2024
Closed
9 tasks
@berland berland force-pushed the eclrun_using_fmstepconfig branch from 33a19a8 to 2cafdf4 Compare October 31, 2024 08:32
@berland berland force-pushed the eclrun_using_fmstepconfig branch 2 times, most recently from 272dc33 to a771236 Compare November 28, 2024 13:19
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (383d645) to head (caa16b6).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9108      +/-   ##
==========================================
- Coverage   91.74%   91.72%   -0.02%     
==========================================
  Files         426      426              
  Lines       26518    26509       -9     
==========================================
- Hits        24328    24316      -12     
- Misses       2190     2193       +3
Flag Coverage Δ
cli-tests 39.86% <86.66%> (+0.15%) ⬆️
everest-models-test 34.23% <53.33%> (+<0.01%) ⬆️
gui-tests 71.97% <40.00%> (+0.02%) ⬆️
integration-test 38.06% <53.33%> (+<0.01%) ⬆️
performance-tests 51.66% <33.33%> (+0.01%) ⬆️
test 39.60% <53.33%> (+<0.01%) ⬆️
unit-tests 74.25% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@berland berland force-pushed the eclrun_using_fmstepconfig branch 2 times, most recently from a71adc2 to 9b2377a Compare December 10, 2024 11:32
@berland berland mentioned this pull request Dec 10, 2024
Closed
9 tasks
@berland berland force-pushed the eclrun_using_fmstepconfig branch from 236da1f to a1452e3 Compare December 11, 2024 09:37
@berland berland force-pushed the eclrun_using_fmstepconfig branch 2 times, most recently from 433974a to a5663d5 Compare December 13, 2024 12:46
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Dec 16, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #9108 will not alter performance

Comparing berland:eclrun_using_fmstepconfig (caa16b6) with main (383d645)

Summary

✅ 24 untouched benchmarks

@berland berland marked this pull request as ready for review December 16, 2024 11:00
@berland berland added the release-notes:refactor PR changes code without changing ANY (!) behavior. label Dec 16, 2024
@berland berland force-pushed the eclrun_using_fmstepconfig branch 5 times, most recently from 92fa474 to 87124b2 Compare December 19, 2024 07:08
@berland berland force-pushed the eclrun_using_fmstepconfig branch 3 times, most recently from 8a627e1 to 9768644 Compare December 27, 2024 09:50
eivindjahren
eivindjahren approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

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

👍

berland
berland commented Jan 3, 2025
View reviewed changes
test-data/everest/egg/everest/model/config_flow.yml
@@ -116,5 +111,5 @@ forward_model:
- well_constraints -i files/well_readydate.json -c files/wc_config.yml -rc well_rate.json -o wc_wells.json
- add_templates -i wc_wells.json -c files/at_config.yml -o at_wells.json
- schmerge -s eclipse/include/schedule/schedule.tmpl -i at_wells.json -o eclipse/include/schedule/schedule.sch
- myflow r{{ eclbase }} --enable-tuning
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, support for arbitrary options forwarded to flow is not included (yet?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now fixed. But --enable-tuning was changed to --enable-tuning=true as the former was not supported by todays version of flow.

@berland berland force-pushed the eclrun_using_fmstepconfig branch from d04533b to 34e64cd Compare January 3, 2025 11:51
@berland
Copy link
Contributor Author

berland commented Jan 3, 2025

TODO: SETENV propagates to the global env in jobs.json but is then overridden by the plugin definition of the same env variables pr forward model step. This must be fixed.

@berland berland force-pushed the eclrun_using_fmstepconfig branch 2 times, most recently from 60affca to 42b2df8 Compare January 6, 2025 07:23
@berland
Copy link
Contributor Author

berland commented Jan 6, 2025

TODO: SETENV propagates to the global env in jobs.json but is then overridden by the plugin definition of the same env variables pr forward model step. This must be fixed.

Fixed.

@berland berland force-pushed the eclrun_using_fmstepconfig branch 4 times, most recently from 0402921 to 3c75afd Compare January 8, 2025 09:20
@berland
Replace erts interface towards reservoir simulators
caa16b6 
This replaces the yaml configuration file for Eclipse100/300 with a set
of environment variables set through the plugin system. Ert cannot any
longer start the raw Eclipse binary itself, it depends on the vendor
supplied wrapper binary called "eclrun".

Similarly, for OPM flow, Ert will now support a wrapper script "flowrun"
if it is present, assuming it has a similar command line API as eclrun.
If flowrun is not present, it will look for a binary "flow" in $PATH
which can be used, but then only with single-cpu possibilities.

Users can point to a custom location of eclrun by adding SETENV to the
configuration file.
@berland berland force-pushed the eclrun_using_fmstepconfig branch from 3c75afd to caa16b6 Compare January 8, 2025 10:52
eivindjahren
eivindjahren approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

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

🧮

sondreso
sondreso approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@sondreso sondreso left a comment

Choose a reason for hiding this comment

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

This is very nice, excellent job! 👏

I think we should add documentation for the forward_model_configuration under https://ert.readthedocs.io/en/latest/getting_started/howto/plugin_system.html, could you create an issue for this? This is fine to address in a separate PR!

@berland berland merged commit 1ae12f6 into equinor:main Jan 9, 2025
43 checks passed
@berland berland mentioned this pull request Jan 9, 2025
Open
@sondreso sondreso mentioned this pull request Jan 9, 2025
Open
@berland berland added the release-notes:breaking-change Automatically categorise as breaking change in release notes label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eivindjahren eivindjahren eivindjahren approved these changes

@sondreso sondreso sondreso approved these changes

Assignees

@berland berland

Labels
release-notes:breaking-change Automatically categorise as breaking change in release notes release-notes:refactor PR changes code without changing ANY (!) behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean up EclRun
4 participants
@berland @codecov-commenter @sondreso @eivindjahren