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

Validate workflow job args against hook runtime fixtures #10341

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yngve-sk
Copy link
Contributor

Issue
Resolves 10334

Approach
Inspect the .run(...) arguments to identify attempts at getting fixtures that are unavailable at the given HookRuntime

@yngve-sk yngve-sk force-pushed the 25.03.17.workflowhooks-add-configvalidation branch from e161baa to 64ec037 Compare March 17, 2025 15:37
Copy link

codspeed-hq bot commented Mar 17, 2025

CodSpeed Performance Report

Merging #10341 will improve performances by 11.75%

Comparing yngve-sk:25.03.17.workflowhooks-add-configvalidation (a1dc63d) with main (0e8c9f2)

Summary

⚡ 1 improvements
✅ 24 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_load_from_context[gen_x: 20, sum_x: 20 reals: 10] 6.5 ms 5.8 ms +11.75%

@yngve-sk yngve-sk force-pushed the 25.03.17.workflowhooks-add-configvalidation branch from 64ec037 to 82530bb Compare March 18, 2025 06:48
@yngve-sk yngve-sk force-pushed the 25.03.17.workflowhooks-add-configvalidation branch from 82530bb to b157fcc Compare March 18, 2025 07:26
Comment on lines 2034 to 2067
@pytest.mark.usefixtures("use_tmpdir")
def test_ert_script_hook_pre_experiment_but_asks_for_ensemble():
workflow_file_path = os.path.join(os.getcwd(), "workflow")
with open(workflow_file_path, mode="w", encoding="utf-8") as fh:
fh.write("TEST_SCRIPT")

with open("config.ert", mode="w", encoding="utf-8") as fh:
fh.write(
dedent(
f"""
NUM_REALIZATIONS 1

LOAD_WORKFLOW {workflow_file_path} workflow_alias
HOOK_WORKFLOW workflow_alias PRE_EXPERIMENT
"""
)
)

class SomeScript(ErtScript):
def run(self, ensemble: LocalEnsemble):
pass

wfjob = ErtScriptWorkflow(
name="TEST_SCRIPT",
ertscript_class=SomeScript,
)
ErtConfig.PREINSTALLED_WORKFLOWS = {"TEST_SCRIPT": wfjob}

with pytest.raises(
ConfigValidationError,
match=r"Workflow job TEST_SCRIPT.*"
r"expected fixtures.*ensemble.*",
):
ErtConfig.from_file("config.ert")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably put almost everything except the class into a fixture?

Comment on lines +14 to +62
fixtures_per_runtime = {
HookRuntime.PRE_EXPERIMENT: {"random_seed"},
HookRuntime.PRE_SIMULATION: {
"storage",
"ensemble",
"reports_dir",
"random_seed",
"run_paths",
},
HookRuntime.POST_SIMULATION: {
"storage",
"ensemble",
"reports_dir",
"random_seed",
"run_paths",
},
HookRuntime.PRE_FIRST_UPDATE: {
"storage",
"ensemble",
"reports_dir",
"random_seed",
"es_settings",
"observation_settings",
"run_paths",
},
HookRuntime.PRE_UPDATE: {
"storage",
"ensemble",
"reports_dir",
"random_seed",
"es_settings",
"observation_settings",
"run_paths",
},
HookRuntime.POST_UPDATE: {
"storage",
"ensemble",
"reports_dir",
"random_seed",
"es_settings",
"observation_settings",
"run_paths",
},
HookRuntime.POST_EXPERIMENT: {
"random_seed",
"storage",
"ensemble",
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we refactor this a bit so that it is easier to keep in sync? Or make sure that the run_models use the same? Think it might be easy for this to come out of sync?

Copy link
Contributor Author

@yngve-sk yngve-sk Mar 18, 2025

Choose a reason for hiding this comment

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

I tried some stuff but I don't think there is a nice solution to it. We would have to have an enumeration over all hooks and their args, and somehow generically make those work across the runmodels, which would require a bigger change, and probably not be sensible with mypy. I tried this:

    def run_workflows_pre_simulation(self, fixtures: PreSimulationFixtures) -> None:
        self.run_workflows(HookRuntime.PRE_SIMULATION, fixtures)

    def run_workflows_post_simulation(self, fixtures: PostSimulationFixtures) -> None:
        self.run_workflows(HookRuntime.POST_SIMULATION, fixtures)

    def run_workflows_pre_first_update(self, fixtures: PreFirstUpdateFixtures) -> None:
        self.run_workflows(HookRuntime.PRE_FIRST_UPDATE, fixtures)

    def run_workflows_pre_update(self, fixtures: PreUpdateFixtures) -> None:
        self.run_workflows(HookRuntime.PRE_UPDATE, fixtures)

    def run_workflows_post_update(self, fixtures: PostUpdateFixtures) -> None:
        self.run_workflows(HookRuntime.POST_UPDATE, fixtures)

    def run_workflows_post_experiment(self, fixtures: PostExperimentFixtures) -> None:
        self.run_workflows(HookRuntime.POST_EXPERIMENT, fixtures)

and this:


class WorkflowFixtures(TypedDict, total=False):
    ensemble: Ensemble | None
    storage: Storage
    random_seed: int | None
    reports_dir: str
    observation_settings: UpdateSettings
    es_settings: ESSettings
    run_paths: Runpaths
    workflow_args: list[Any]
    parent: QWidget | None


class PreExperimentFixtures(WorkflowFixtures, total=False):
    random_seed: int | None


class PreSimulationFixtures(WorkflowFixtures, total=False):
    storage: Storage
    ensemble: Ensemble | None
    reports_dir: str
    random_seed: int | None
    run_paths: Runpaths


PostSimulationFixtures = PreSimulationFixtures


class PreFirstUpdateFixtures(WorkflowFixtures, total=False):
    storage: Storage
    ensemble: Ensemble|None
    reports_dir: str
    random_seed: int | None
    run_paths: Runpaths


PreUpdateFixtures = PreFirstUpdateFixtures
PostUpdateFixtures = PreFirstUpdateFixtures


class PostExperimentFixtures(WorkflowFixtures, total=False):
    random_seed: int | None

This is OK for mypy, but then to get the enumeration we would have to get the keys of the typeddict subclass, to get the list of accepted fixtures, and we'd also still have to maintain the mapping. I think this can be done with some "meta" style programming but I think it will add more complexity than it removes.

@yngve-sk yngve-sk force-pushed the 25.03.17.workflowhooks-add-configvalidation branch from fdb774f to a1dc63d Compare March 18, 2025 13:49
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.

Add config validation for invalid HOOK_RUNTIME workflows
2 participants