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

Fix DESIGN_KW validation flag bug #649

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

jonathan-eq
Copy link
Contributor

This commit fixes the issue where the <VALIDATE>=true argument would be parsed as a positional argument instead of a keyword argument, and the arg parsed would raise an exception for unexpected argument false. After this commit, the argument is only for ert validation purposes, and is not forwarded to the DESIGN_KW argparser.

@jonathan-eq
Copy link
Contributor Author

Is there a good way of documenting that the option can still be set in the ert config?

@jonathan-eq jonathan-eq self-assigned this Oct 7, 2024
@jonathan-eq jonathan-eq added the bug Something isn't working label Oct 7, 2024
@eivindjahren
Copy link
Contributor

It is really important one can set the <VALIDATE> keyword in FORWARD_MODEL DESIGN_KW(..., how is it that one does that now? Also, why didn't the default value work? It is supposed to work.

@jonathan-eq
Copy link
Contributor Author

It is really important one can set the <VALIDATE> keyword in FORWARD_MODEL DESIGN_KW(..., how is it that one does that now? Also, why didn't the default value work? It is supposed to work.

It is still the same, but the value is never passed to the DESIGN_KW script, it is only retrieved in the validate_forward_model_config step. The default value was passed to the script as a positional argument (without the --validate=-part) , which is why we got the error

@eivindjahren
Copy link
Contributor

Then it should be possible to use

        super().__init__(
            name="DESIGN_KW",
            command=["design_kw", "<template_file>", "<result_file>", "--validate=<VALIDATE>"],
            default_mapping={"<VALIDATE>": "false"},
        )

@jonathan-eq
Copy link
Contributor Author

Then it should be possible to use

        super().__init__(
            name="DESIGN_KW",
            command=["design_kw", "<template_file>", "<result_file>", "--validate=<VALIDATE>"],
            default_mapping={"<VALIDATE>": "false"},
        )

Yes, but do we want to pass it to the script/have it as a script argument if it is unused?

@eivindjahren
Copy link
Contributor

Then it should be possible to use

        super().__init__(
            name="DESIGN_KW",
            command=["design_kw", "<template_file>", "<result_file>", "--validate=<VALIDATE>"],
            default_mapping={"<VALIDATE>": "false"},
        )

Yes, but do we want to pass it to the script/have it as a script argument if it is unused?

Yes, that seems unproblematic.

This commit fixes the issue where the `<VALIDATE>=true` argument would be parsed as a positional argument instead of a keyword argument, and the arg parsed would raise an exception for unexpected argument `false`.
After this commit, the <VALIDATE> argument is only for ert validation purposes, and is not forwarded to the `DESIGN_KW` argparser.
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.

👍

@jonathan-eq jonathan-eq merged commit eae1c85 into equinor:main Oct 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants