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

Raise error on invalid use of runpath placeholders #6731

Conversation

andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Nov 30, 2023

Issue
Resolves #6617

According to FMU-docs:

Deprecated syntax still allow use of two %d specifers. Use of less than two %d is prohibited. The behaviour is identical to the default substitution.

https://fmu-docs.equinor.com/docs/ert/reference/configuration/keywords.html#runpath

Pre review checklist

  • Read through the code changes carefully after finishing work
  • Make sure tests pass locally (after every commit!)
  • Prepare changes in small commits for more convenient review (optional)
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Updated documentation
  • Ensured that unit tests are added for all new behavior (See
    Ground Rules),
    and changes to existing code have good test coverage.

Pre merge checklist

  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.

@andreas-el andreas-el requested a review from larsevj November 30, 2023 13:58
@andreas-el andreas-el self-assigned this Nov 30, 2023
@andreas-el andreas-el added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Nov 30, 2023
@andreas-el andreas-el force-pushed the assert_none_or_two_placeholders_for_runpath branch from 59489d0 to 91ac616 Compare November 30, 2023 14:17
Copy link
Contributor

@larsevj larsevj left a comment

Choose a reason for hiding this comment

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

LGTM! Just need to change the deprecation test.

[
("simulations/realizations-<IENS>/iter-<ITER>", False),
("simulations/realizations-<IENS>/iter-%d", True),
("simulations/realizations-%d", True),
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a breaking change. Running an ensemble experiment on poly with this path works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a conflict with respect to what the docs state and what the code previously allowed.
Which of these are correct is obviously up for debate.

Docs state;

Use of less than two %d is prohibited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem detected was mixing the deprecated %d syntax, with the newer <IENS> and <ITER> placeholders. We could allow one %d but at the same time rather check we don't mix the two styles.

Would that be an acceptable solution?

Copy link
Contributor

@berland berland Nov 30, 2023

Choose a reason for hiding this comment

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

The docs are not in accordance with how ert is in use. Not using <ITER> in RUNPATH is normal when doing e.g. prediction runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's update the docs too then.

Using one %d or just <IENS> is okay, but mixing them is not.
Also more than two %d's does also not make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented as such.

@andreas-el andreas-el force-pushed the assert_none_or_two_placeholders_for_runpath branch from 91ac616 to a757d71 Compare November 30, 2023 14:30
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86a31ef) 83.56% compared to head (a757d71) 83.57%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6731      +/-   ##
==========================================
+ Coverage   83.56%   83.57%   +0.01%     
==========================================
  Files         359      359              
  Lines       20952    20956       +4     
  Branches      948      948              
==========================================
+ Hits        17509    17515       +6     
+ Misses       3149     3147       -2     
  Partials      294      294              

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

@andreas-el andreas-el requested a review from berland November 30, 2023 14:54
@andreas-el andreas-el force-pushed the assert_none_or_two_placeholders_for_runpath branch 3 times, most recently from b4899ed to 883d65a Compare December 1, 2023 11:21
@andreas-el andreas-el requested a review from larsevj December 1, 2023 11:21
@andreas-el andreas-el force-pushed the assert_none_or_two_placeholders_for_runpath branch from 883d65a to c6a11e4 Compare December 1, 2023 11:22
Copy link
Contributor

@larsevj larsevj left a comment

Choose a reason for hiding this comment

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

Is this Error message a little too generic? It might be difficult to figure out what you did wrong? Especially if we are still supporting other types of runpath, than the DEFAULT one?

@andreas-el andreas-el requested review from larsevj December 1, 2023 12:00
@andreas-el andreas-el force-pushed the assert_none_or_two_placeholders_for_runpath branch from c4e9699 to 214c045 Compare December 1, 2023 12:00
@andreas-el
Copy link
Contributor Author

Is this Error message a little too generic? It might be difficult to figure out what you did wrong? Especially if we are still supporting other types of runpath, than the DEFAULT one?

That's a good point. Lets make them more specific to convey more information.

Copy link
Contributor

@larsevj larsevj left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@andreas-el andreas-el merged commit b2d1b2c into equinor:main Dec 1, 2023
37 of 41 checks passed
@andreas-el andreas-el deleted the assert_none_or_two_placeholders_for_runpath branch December 1, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ert suggests wrong Runpath placeholder keyword
4 participants