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

Make it possible for plugins to subtype ForwardModel #7409

Closed
eivindjahren opened this issue Mar 8, 2024 · 3 comments · Fixed by #7743
Closed

Make it possible for plugins to subtype ForwardModel #7409

eivindjahren opened this issue Mar 8, 2024 · 3 comments · Fixed by #7743
Assignees
Labels
configuration improvement Something nice to have, that will make life easier for developers or users or both.

Comments

@eivindjahren
Copy link
Contributor

Currently, making e.g. pyscal form semeio available as a forward model in ert means creating a config file, and pointing ert to it with the plugin interface method installable_jobs() (done here).

From there, it is inserted into the site config and read here:

for job in config_dict.get(ConfigKeys.INSTALL_JOB, []):
name = job[0]
job_config_file = path.abspath(job[1])
try:
new_job = ForwardModel.from_config_file(
name=name,
config_file=job_config_file,
)
except ConfigValidationError as e:
errors.append(e)
continue
if name in jobs:
ConfigWarning.ert_context_warn(
f"Duplicate forward model job with name {name!r}, choosing "
f"{job_config_file!r} over {jobs[name].executable!r}",
name,
)
jobs[name] = new_job
for job_path in config_dict.get(ConfigKeys.INSTALL_JOB_DIRECTORY, []):
for file_name in _get_files_in_directory(job_path, errors):
if not path.isfile(file_name):
continue
try:
new_job = ForwardModel.from_config_file(config_file=file_name)

This makes it difficult to add additional validation of the setup such as valid version number etc.

In order to remedy this, the suggestion is that the plugin interface is extended so that e.g. fm_pyscal can be implemented as a subtype of ForwardModel. The specifics of a forward model read from a config file would have to also be pushed out into subtype of ForwardModel e.g. ConfigFileForwardModel, so that ForwardModel is an abstract baseclass.

In this way the PYSCAL forward model could be implemented like this:

class Pyscal(ForwardModel):
    def __init__(self):
        super().__init__(
            name="PYSCAL",
            executable="fm_pyscal",
        )

    def create_arguments(self, **parameters:str) -> List[str]:
        ...

where create_arguments is responsible for handling default values and validating parameters.

@eivindjahren eivindjahren added configuration needs-discussion Issues requiring further discussions improvement Something nice to have, that will make life easier for developers or users or both. labels Mar 8, 2024
@sondreso
Copy link
Collaborator

I think this looks like a good approach! I think we can make a draft based on this suggestion and discuss based on that.

One thing that isn't mentioned in the text above is that it is important that there is a good way for plugins to report configuration errors, and that they show up in the GUI the same way as the errors from normal config parsing.

@sondreso sondreso removed the needs-discussion Issues requiring further discussions label Apr 22, 2024
@sondreso
Copy link
Collaborator

We also need to setup plugin hooks for the plugins to register subtypes of ForwardModel

@eivindjahren
Copy link
Contributor Author

eivindjahren commented Apr 24, 2024

@yngve-sk Just got word that the typical use-case is that e.g. the ECLIPSE100 forward model step would like to check that the version given is installed so that

        FORWARD_MODEL ECLIPSE100(<VERSION>=smersion, <NUM_CPU>=42)

fails the custom argument list check as there is version of the eclipse simulator installed called "smersion".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration improvement Something nice to have, that will make life easier for developers or users or both.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants