-
Notifications
You must be signed in to change notification settings - Fork 110
Move GEN_KW template feature into RUN_TEMPLATE functionality #10549
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
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #10549 will not alter performanceComparing Summary
|
d4f4549
to
6f7dea0
Compare
@@ -103,6 +103,16 @@ def _check_for_forward_init_in_gen_kw(gen_kw_list: list[GenKwConfig]) -> None: | |||
if gen_kw.forward_init_file is not None: | |||
logger.info(f"GEN_KW uses FORWARD_INIT: {gen_kw}") | |||
|
|||
@no_type_check |
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.
Is it risky to use this decorator?
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.
I've used it since it was used in from_config_list from before. But I can try without.
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.
It needs to be there due to config_dict handling (ie. config_dict.get(...)
)
gen_kw_key = cast(str, gen_kw[0]) | ||
positional_args = cast(list[str], gen_kw[:-1]) | ||
|
||
if len(positional_args) == 4: |
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.
Can we change this to if len(positional_args) != 4: return None
to avoid some of the indentation?
src/ert/storage/local_storage.py
Outdated
) | ||
return None | ||
|
||
elif version < _LOCAL_STORAGE_VERSION: | ||
migrations = list( | ||
enumerate([to2, to3, to4, to5, to6, to7, to8, to9], start=1) | ||
enumerate([to2, to3, to4, to5, to6, to7, to8, to9, to10], start=1) |
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.
What is this? Would it be the same to do [to2,to3,to4,to5,to6...][1:] instead of list(enumerate(inner_list))?
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.
This one does the migrations starting from version-1 up to version-10. I guess nobody really wanted to change it since the early days. For the sake of least number of changes, I've just added the necessary migration.
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.
I have some questions, and some nitpicking 🥇
8b36e57
to
2e9ce24
Compare
I think you can drop the |
@@ -315,6 +313,7 @@ def read_templates(config_dict) -> list[tuple[str, str]]: | |||
"it is synced with your DATA file." | |||
) | |||
templates.append(template) | |||
templates.extend(EnsembleConfig.get_gen_kw_templates(config_dict)) |
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.
Are we testing this?
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.
no as it is a very temporal change, I was not planning to 😁 But maybe we should 🤷
This includes adding parameter substition feature into substitute class. GenKwConfig.templates_from_config is reponsible for extracting tmpl and output files. In case an absolute path is provided as an output file it raises ConfigValidationError. To prevent multiple storage migrations this also remove forward_init from gen_kw.
This PR does a few things to prevent several storage migrations:
to10.py
storage migrationIssue
Resolves #10180
Resolves #10596
Approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests'
)When applicable