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

Add post/pre experiment simulation hooks #8993

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

yngve-sk
Copy link
Contributor

Issue
Resolves #7644

Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Looks good, but think we need to adjust the timing a bit. Also it seems to be missing for some experiment types. Though we should not run the PRE_EXPERIMENT hooks for restart runs and evaluate_experiment´ and none of them for manual_update. Also need to add documentation for when these are run, and for which run model. There is another test: test_model_hook_order` which should proably assert this.

src/ert/run_models/ensemble_experiment.py Outdated Show resolved Hide resolved
src/ert/run_models/ensemble_experiment.py Outdated Show resolved Hide resolved
src/ert/run_models/ensemble_experiment.py Outdated Show resolved Hide resolved
@yngve-sk yngve-sk force-pushed the add-post-experiment-hook branch 2 times, most recently from 155e4cd to 687513a Compare November 25, 2024 13:59
@yngve-sk yngve-sk marked this pull request as ready for review November 25, 2024 14:09
@yngve-sk yngve-sk force-pushed the add-post-experiment-hook branch 2 times, most recently from 390eb0b to 3801742 Compare November 26, 2024 07:29
@yngve-sk yngve-sk requested a review from oyvindeide November 27, 2024 08:07
@yngve-sk yngve-sk force-pushed the add-post-experiment-hook branch from bb33887 to 5b3d243 Compare November 29, 2024 10:04
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Looks good, think the test can be improved a bit though


LOAD_WORKFLOW_JOB SAY_HELLO_POST_EXP dump_final_ensemble_id
LOAD_WORKFLOW SAY_HELLO_POST_EXP.wf POST_EXPERIMENT_DUMP
HOOK_WORKFLOW POST_EXPERIMENT_DUMP POST_EXPERIMENT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could parameterize PRE and POST so we dont check two different (though related) things in the same test. Think it will shorten the test quite a bit also. Could also parameterize the output if you want different for the two hooks. Should also be possible to assert on the stdout if you just print in the scripts: https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html#accessing-captured-output-from-a-test-function

Comment on lines 560 to 625
@pytest.mark.usefixtures("copy_poly_case")
def test_that_pre_post_experiment_hook_works(
monkeypatch,
):
monkeypatch.setattr(_ert.threading, "_can_raise", False)

# The executable
with open("hello_post_exp.sh", "w", encoding="utf-8") as f:
f.write(
dedent("""#!/bin/bash
echo "just sending regards" > from_post_experiment.txt
""")
)
os.chmod("hello_post_exp.sh", 0o755)

# The workflow job
with open("SAY_HELLO_POST_EXP", "w", encoding="utf-8") as s:
s.write("""
INTERNAL False
EXECUTABLE hello_post_exp.sh
""")

# The workflow
with open("SAY_HELLO_POST_EXP.wf", "w", encoding="utf-8") as s:
s.write("""dump_final_ensemble_id""")

# The executable
with open("hello_pre_exp.sh", "w", encoding="utf-8") as f:
f.write(
dedent("""#!/bin/bash
echo "first" > from_pre_experiment.txt
""")
)
os.chmod("hello_pre_exp.sh", 0o755)

# The workflow job
with open("SAY_HELLO_PRE_EXP", "w", encoding="utf-8") as s:
s.write("""
INTERNAL False
EXECUTABLE hello_pre_exp.sh
""")

# The workflow
with open("SAY_HELLO_PRE_EXP.wf", "w", encoding="utf-8") as s:
s.write("""dump_first_ensemble_id""")

with open("poly.ert", mode="a", encoding="utf-8") as fh:
fh.write(
dedent(
"""
NUM_REALIZATIONS 2

LOAD_WORKFLOW_JOB SAY_HELLO_POST_EXP dump_final_ensemble_id
LOAD_WORKFLOW SAY_HELLO_POST_EXP.wf POST_EXPERIMENT_DUMP
HOOK_WORKFLOW POST_EXPERIMENT_DUMP POST_EXPERIMENT

LOAD_WORKFLOW_JOB SAY_HELLO_PRE_EXP dump_first_ensemble_id
LOAD_WORKFLOW SAY_HELLO_PRE_EXP.wf PRE_EXPERIMENT_DUMP
HOOK_WORKFLOW PRE_EXPERIMENT_DUMP PRE_EXPERIMENT
"""
)
)

run_cli(ITERATIVE_ENSEMBLE_SMOOTHER_MODE, "--disable-monitor", "poly.ert")

assert (Path(os.getcwd()) / "from_pre_experiment.txt").read_text(
"utf-8"
) == "first\n"
assert (Path(os.getcwd()) / "from_post_experiment.txt").read_text(
"utf-8"
) == "just sending regards\n"


Copy link
Collaborator

Choose a reason for hiding this comment

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

For example:

Suggested change
@pytest.mark.usefixtures("copy_poly_case")
def test_that_pre_post_experiment_hook_works(
monkeypatch,
):
monkeypatch.setattr(_ert.threading, "_can_raise", False)
# The executable
with open("hello_post_exp.sh", "w", encoding="utf-8") as f:
f.write(
dedent("""#!/bin/bash
echo "just sending regards" > from_post_experiment.txt
""")
)
os.chmod("hello_post_exp.sh", 0o755)
# The workflow job
with open("SAY_HELLO_POST_EXP", "w", encoding="utf-8") as s:
s.write("""
INTERNAL False
EXECUTABLE hello_post_exp.sh
""")
# The workflow
with open("SAY_HELLO_POST_EXP.wf", "w", encoding="utf-8") as s:
s.write("""dump_final_ensemble_id""")
# The executable
with open("hello_pre_exp.sh", "w", encoding="utf-8") as f:
f.write(
dedent("""#!/bin/bash
echo "first" > from_pre_experiment.txt
""")
)
os.chmod("hello_pre_exp.sh", 0o755)
# The workflow job
with open("SAY_HELLO_PRE_EXP", "w", encoding="utf-8") as s:
s.write("""
INTERNAL False
EXECUTABLE hello_pre_exp.sh
""")
# The workflow
with open("SAY_HELLO_PRE_EXP.wf", "w", encoding="utf-8") as s:
s.write("""dump_first_ensemble_id""")
with open("poly.ert", mode="a", encoding="utf-8") as fh:
fh.write(
dedent(
"""
NUM_REALIZATIONS 2
LOAD_WORKFLOW_JOB SAY_HELLO_POST_EXP dump_final_ensemble_id
LOAD_WORKFLOW SAY_HELLO_POST_EXP.wf POST_EXPERIMENT_DUMP
HOOK_WORKFLOW POST_EXPERIMENT_DUMP POST_EXPERIMENT
LOAD_WORKFLOW_JOB SAY_HELLO_PRE_EXP dump_first_ensemble_id
LOAD_WORKFLOW SAY_HELLO_PRE_EXP.wf PRE_EXPERIMENT_DUMP
HOOK_WORKFLOW PRE_EXPERIMENT_DUMP PRE_EXPERIMENT
"""
)
)
run_cli(ITERATIVE_ENSEMBLE_SMOOTHER_MODE, "--disable-monitor", "poly.ert")
assert (Path(os.getcwd()) / "from_pre_experiment.txt").read_text(
"utf-8"
) == "first\n"
assert (Path(os.getcwd()) / "from_post_experiment.txt").read_text(
"utf-8"
) == "just sending regards\n"
@pytest.mark.parametrize("hook", ["PRE", "POST"])
def test_that_pre_post_experiment_hook_works(
monkeypatch, tmp_path, hook, capsys
):
monkeypatch.chdir(tmp_path)
monkeypatch.setattr(_ert.threading, "_can_raise", False)
# The executable
with open("hello.sh", "w", encoding="utf-8") as f:
f.write(
dedent("""#!/bin/bash
echo "just sending regards"
""")
)
os.chmod("hello.sh", 0o755)
# The workflow job
with open("SAY_HELLO", "w", encoding="utf-8") as s:
s.write("""
INTERNAL False
EXECUTABLE hello.sh
""")
# The workflow
with open("SAY_HELLO_CONFIG", "w", encoding="utf-8") as s:
s.write("""SAY_HELLO""")
with open("poly.ert", mode="a", encoding="utf-8") as fh:
fh.write(
dedent(
f"""
NUM_REALIZATIONS 2
LOAD_WORKFLOW_JOB SAY_HELLO
LOAD_WORKFLOW SAY_HELLO_CONFIG
HOOK_WORKFLOW SAY_HELLO_CONFIG {hook.upper()}_EXPERIMENT
"""
)
)
run_cli(ENSEMBLE_EXPERIMENT_MODE, "--disable-monitor", "poly.ert")
captured = capsys.readouterr()
assert "just sending regards" in captured.out

could also look at: test_model_hook_order, so it can be tested for all relevant experiments, and not just ies

Copy link
Contributor Author

@yngve-sk yngve-sk Jan 6, 2025

Choose a reason for hiding this comment

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

I think the runtime of the test will be quite long if we run it for all modes, we also need observations to run anything beyond ensemble experiment so I think the copy poly (or creating observations, having params and running a forward model + responses is needed). Running with parametrized PRE/POST with ensemble_experiment (and copy poly) took 54s for me locally, so I think maybe keeping it verbose and for ES only might be the way. We test that the hooks are invoked in test_model_hook_order. Running for all modes with capsys without parametrizing PRE/POST takes 47s for me locally.

running the test below took ~5min locally


@pytest.mark.usefixtures("copy_poly_case")
@pytest.mark.parametrize("hook", ["PRE", "POST"])
def test_that_pre_post_experiment_hook_works(monkeypatch, hook, capsys):
    monkeypatch.setattr(_ert.threading, "_can_raise", False)
    # The executable
    with open("hello.sh", "w", encoding="utf-8") as f:
        f.write(
            dedent("""#!/bin/bash
                echo "just sending regards"
        """)
        )
    os.chmod("hello.sh", 0o755)
    # The workflow job
    with open("SAY_HELLO", "w", encoding="utf-8") as s:
        s.write("""
               INTERNAL False
               EXECUTABLE hello.sh
           """)
    # The workflow
    with open("SAY_HELLO_CONFIG", "w", encoding="utf-8") as s:
        s.write("""SAY_HELLO""")

    with open("poly.ert", mode="a", encoding="utf-8") as fh:
        fh.write(
            dedent(
                f"""
                    LOAD_WORKFLOW_JOB SAY_HELLO
                    LOAD_WORKFLOW SAY_HELLO_CONFIG
                    HOOK_WORKFLOW SAY_HELLO_CONFIG {hook.upper()}_EXPERIMENT
                """
            )
        )

    for mode in [
        ENSEMBLE_EXPERIMENT_MODE,
        ENSEMBLE_SMOOTHER_MODE,
        ITERATIVE_ENSEMBLE_SMOOTHER_MODE,
        ES_MDA_MODE,
    ]:
        run_cli(mode, "--disable-monitor", "poly.ert")
        captured = capsys.readouterr()
        assert "just sending regards" in captured.out

Copy link

codspeed-hq bot commented Jan 6, 2025

CodSpeed Performance Report

Merging #8993 will not alter performance

Comparing yngve-sk:add-post-experiment-hook (9637946) with main (864c1e8)

Summary

✅ 24 untouched benchmarks

@yngve-sk yngve-sk force-pushed the add-post-experiment-hook branch from cc878e0 to eab7674 Compare January 6, 2025 09:27
@yngve-sk yngve-sk force-pushed the add-post-experiment-hook branch from eab7674 to 9637946 Compare January 6, 2025 09:39
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (864c1e8) to head (9637946).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8993      +/-   ##
==========================================
- Coverage   91.73%   91.72%   -0.01%     
==========================================
  Files         430      430              
  Lines       26639    26649      +10     
==========================================
+ Hits        24436    24443       +7     
- Misses       2203     2206       +3     
Flag Coverage Δ
cli-tests 39.71% <100.00%> (+0.02%) ⬆️
everest-models-test 34.18% <38.46%> (-0.01%) ⬇️
gui-tests 71.84% <84.61%> (+<0.01%) ⬆️
integration-test 38.29% <38.46%> (-0.03%) ⬇️
performance-tests 51.69% <69.23%> (+<0.01%) ⬆️
test 39.61% <38.46%> (-0.03%) ⬇️
unit-tests 74.00% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@yngve-sk yngve-sk added the release-notes:new-feature Automatically categorise as new feature in release notes label Jan 6, 2025
@yngve-sk yngve-sk merged commit d24f588 into equinor:main Jan 6, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:new-feature Automatically categorise as new feature in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new workflow hooks, PRE and POST experiment
4 participants