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

Instantiate ert config before starting everest server #9244

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

DanSava
Copy link
Contributor

@DanSava DanSava commented Nov 18, 2024

Issue
Resolves #9200

Approach
Instantiate ert_confg from everest config before we start everest server and report expectations as config validation error.

  • 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

Copy link
Contributor

@StephanDeHoop StephanDeHoop left a comment

Choose a reason for hiding this comment

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

Two small questions/comments, for the rest looks good to me. Also, I haven't checked out the code and somehow fail to search for it in the checks/pipeline (if you have a tip on how to search it, please let me know :p), but does this test ("test_starting_not_in_folder") now fail since we do validation earlier or did I misunderstood the discussion on the issue?


is_dir.append(os.path.isdir(geo_source))

if not is_dir:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Or am I missing something (or is it a better safe than sorry)?

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 fixing a small bug I noticed while testing out the functionality. But I guess it should not be part of this PR I will add it in a separate PR.

src/everest/simulator/everest_to_ert.py Show resolved Hide resolved
@DanSava
Copy link
Contributor Author

DanSava commented Nov 19, 2024

Two small questions/comments, for the rest looks good to me. Also, I haven't checked out the code and somehow fail to search for it in the checks/pipeline (if you have a tip on how to search it, please let me know :p), but does this test ("test_starting_not_in_folder") now fail since we do validation earlier or did I misunderstood the discussion on the issue?

test_starting_not_in_folder will not fail if we do the validation earlier. That one test does not really start the everest server it is mocking the behaviour and will not be affected by this change.

Also, you are having trouble finding the tests because it is an integration tests and those tests are run using this command pytest tests/everest -n 4 -m "integration_test" --dist loadgroup (from .github/workflows/test_everest.yml) and this will not output individual test results from the files that have run. Maybe we should consider having a more verbose output for the integration tests as well.

@DanSava DanSava force-pushed the validate-ert-config-upfront branch from 4a2415d to d9069f2 Compare November 19, 2024 09:25
@DanSava DanSava self-assigned this Nov 19, 2024
@DanSava DanSava merged commit 0d2c4c8 into equinor:main Nov 19, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration not fully validated before starting the experiment
2 participants