Refactor control handling in everest config #9805

verveerpj · 2025-01-20T10:54:59Z

Issue
The parsing code in everest2ropt for the controls is extremely convoluted, due to the nested character of the controls section in the configuration. This PR simplifies the parsing code by creating an intermediate class that stores the controls and its properties in a linear fashion. The relevant code in everest2ropt is rewritten accordingly, becoming much more easy to understand.

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

PR title captures the intent of the changes, and is fitting for release notes.
Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.
Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n auto -m "not integration_test"')

When applicable

When there are user facing changes: Updated documentation
New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
Large PR: Prepare changes in small commits for more convenient review
Bug fix: Add regression test for the bug
Bug fix: Create Backport PR to latest release

codecov-commenter · 2025-01-20T10:58:26Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2849	1	2848	117

View the full list of 1 ❄️ flaky tests

tests/ert/ui_tests/cli/test_cli.py::test_that_pre_post_experiment_hook_works

Flake rate in main: 7.69% (Passed 96 times, Failed 8 times)

Stack Traces | 12.2s run time

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fbcc69fd280>
capsys = <_pytest.capture.CaptureFixture object at 0x7fbcc69ffda0>

    @pytest.mark.usefixtures("copy_poly_case")
    def test_that_pre_post_experiment_hook_works(monkeypatch, capsys):
        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"
            """)
            )
        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"
            """)
            )
        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
                    """
                )
            )
    
        for mode in [ITERATIVE_ENSEMBLE_SMOOTHER_MODE, ES_MDA_MODE, ENSEMBLE_SMOOTHER_MODE]:
>           run_cli(mode, "--disable-monitoring", "poly.ert")

.../ui_tests/cli/test_cli.py:585: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../ui_tests/cli/run_cli.py:14: in run_cli
    res = cli_runner(parsed)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = Namespace(logdir='./logs', read_only=False, mode='iterative_ensemble_smoother', current_ensemble='default', target_ens... disable_monitoring=True, port_range=range(49152, 51819), config='poly.ert', func=<function run_cli at 0x7fbcc4563060>)
plugin_manager = None

    def run_cli(args: Namespace, plugin_manager: ErtPluginManager | None = None) -> None:
        ert_dir = os.path.abspath(os.path.dirname(args.config))
        os.chdir(ert_dir)
        # Changing current working directory means we need to update
        # the config file to be the base name of the original config
        args.config = os.path.basename(args.config)
    
        ert_config = ErtConfig.with_plugins().from_file(args.config)
    
        local_storage_set_ert_config(ert_config)
        counter_fm_steps = Counter(fms.name for fms in ert_config.forward_model_steps)
    
        # Create logger inside function to make sure all handlers have been added to
        # the root-logger.
        logger = logging.getLogger(__name__)
        for fm_step_name, count in counter_fm_steps.items():
            logger.info(
                f"Config contains forward model step {fm_step_name} {count} time(s)",
            )
    
        if not ert_config.observations and args.mode not in {
            ENSEMBLE_EXPERIMENT_MODE,
            TEST_RUN_MODE,
            WORKFLOW_MODE,
        }:
            raise ErtCliError(
                f"To run {args.mode}, observations are needed. \n"
                f"Please add an observation file to {args.config}. Example: \n"
                f"'OBS_CONFIG observation_file.txt'."
            )
    
        if args.mode in {
            ENSEMBLE_SMOOTHER_MODE,
            ES_MDA_MODE,
            ITERATIVE_ENSEMBLE_SMOOTHER_MODE,
        }:
            if not ert_config.ensemble_config.parameter_configs:
                raise ErtCliError(
                    f"To run {args.mode}, GEN_KW, FIELD or SURFACE parameters are needed. \n"
                    f"Please add to file {args.config}"
                )
            if not any(
                p.update for p in ert_config.ensemble_config.parameter_configs.values()
            ):
                raise ErtCliError(
                    f"All parameters are set to UPDATE:FALSE in {args.config}"
                )
    
        storage = open_storage(ert_config.ens_path, "w")
    
        if args.mode == WORKFLOW_MODE:
            execute_workflow(ert_config, storage, args.name)
            return
    
        status_queue: queue.SimpleQueue[StatusEvents] = queue.SimpleQueue()
        try:
            model = create_model(
                ert_config,
                storage,
                args,
                status_queue,
            )
        except ValueError as e:
            raise ErtCliError(f"{args.mode} was not valid, failed with: {e}") from e
    
        if args.port_range is None and model.queue_system == QueueSystem.LOCAL:
            # This is within the range for ephemeral ports as defined by
            # most unix flavors https://en.wikipedia.org/wiki/Ephemeral_port
            args.port_range = range(49152, 51819)
    
        use_ipc_protocol = model.queue_system == QueueSystem.LOCAL
        evaluator_server_config = EvaluatorServerConfig(
            custom_port_range=args.port_range, use_ipc_protocol=use_ipc_protocol
        )
    
        if model.check_if_runpath_exists():
            print(
                "Warning: ERT is running in an existing runpath.\n"
                "Please be aware of the following:\n"
                "- Previously generated results "
                "might be overwritten.\n"
                "- Previously generated files might "
                "be used if not configured correctly.\n"
                f"- {model.get_number_of_existing_runpaths()} out of {model.get_number_of_active_realizations()} realizations "
                "are running in existing runpaths.\n"
            )
            logger.warning("ERT is running in an existing runpath")
    
        thread = ErtThread(
            name="ert_cli_simulation_thread",
            target=model.start_simulations_thread,
            args=(evaluator_server_config,),
        )
    
        with contextlib.ExitStack() as exit_stack:
            out: TextIO
            if args.disable_monitoring:
                out = exit_stack.enter_context(open(os.devnull, "w", encoding="utf-8"))
            else:
                out = sys.stderr
            monitor = Monitor(out=out, color_always=args.color_always)
            thread.start()
            end_event: EndEvent | None = None
            try:
                end_event = monitor.monitor(
                    status_queue, ert_config.analysis_config.log_path
                )
            except (SystemExit, KeyboardInterrupt, OSError):
                print("\nKilling simulations...")
                model.cancel()
    
        thread.join()
        storage.close()
    
        if end_event is not None and end_event.failed:
            # If monitor has not reported, give some info if the job failed
            msg = end_event.msg if args.disable_monitoring else ""
>           raise ErtCliError(msg)
E           ert.cli.main.ErtCliError:

.../hostedtoolcache/Python/3.12.8.../x64/lib/python3.12.../ert/cli/main.py:154: ErtCliError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

codspeed-hq · 2025-01-20T11:26:28Z

CodSpeed Performance Report

Merging #9805 will not alter performance

_{Comparing new-control-parsing (4d44cab) with main (15109bb)}

Summary

✅ 24 untouched benchmarks

StephanDeHoop

Looks good to me! everest2ropt is indeed much cleaner like this. I have some minor comments that you might want to look at before merging.

StephanDeHoop · 2025-01-21T19:49:33Z

src/everest/config/everest_config.py

    @property
    def objective_names(self) -> list[str]:
        return [objective.name for objective in self.objective_functions]

-    @cached_property
+    @property


Just wondering, method seems unchanged and result doesn't seem to change, is there a reason we remove caching here?

At some point I ran into an issue where @cached_property did not work properly. EverestConfig is not immutable and if you change it afterwards by assigning to it the cache is not updated. So that is a bit dangerous. There is currently an effort to make it immutable again, then we could use @cached_property.

StephanDeHoop · 2025-01-21T19:53:45Z

src/everest/config/utils.py

+                        var_dict["sampler_idx"] = len(self._samplers) - 1
+                    else:
+                        if control.sampler is not None and control_sampler_idx < 0:
+                            self._samplers.append(control.sampler)


Curious code complexity is not checked for this (I guess pre-commit doesn't do this or there is nothing related to this that's checked), but this nesting (of if- and for-code blocks) seems quite excessive in some parts, we could split some of it up for readability, but maybe it's not worth it? Just wondering what you think!

It is indeed a lot of nesting, I will have a look to reduce that.

StephanDeHoop · 2025-01-21T19:57:26Z

src/everest/config/utils.py

+                        if control.sampler is not None and control_sampler_idx < 0:
+                            self._samplers.append(control.sampler)
+                            control_sampler_idx = len(self._samplers) - 1
+                        var_dict["sampler_idx"] = control_sampler_idx


Does it ever happen that control.sampler is not None when the variable also doesn't have it's own sampler (I guess not)?

Maybe I misunderstand the question, but it works like this:

If no variable sampler is given, the control sampler will be used

If no variable sampler is given and no controls sampler is given, that variable will not be perturbed

If no samplers are given at all anywhere, a default sampler is used for everything.

Ah I see, thanks :)!

StephanDeHoop · 2025-01-21T20:01:43Z

src/everest/optimizer/everest2ropt.py

        return

+    formatted_names = [
+        (
+            f"{control_name[0]}.{control_name[1]}-{control_name[2]}"


We have this logic twice (depending on the control_name size), is it worth making a separate class out of it (to have the logic in one place) that has a __str__ which we could use here? Maybe not necessary.

Or maybe just have it as a property of everest config.

I agree, but I think we should do this in the context of this issue: #9816

That issue points out that the formatting is not consistent, and for input constraints it is defined here. When that issue is resolved, it could become a property of the config.

DanSava · 2025-01-22T06:52:00Z

src/everest/optimizer/everest2ropt.py

-    formatted_names,
-):
-    if not input_constraints:
+def _parse_input_constraints(ever_config: EverestConfig, ropt_config):


I remember there was an issue some time back refactoring code to avoid using the full Everest config as function arguments. Is that not still an issue?

I don't know if it is an issue, but since that seems to be our policy now, I will fix it.

verveerpj · 2025-01-22T08:19:00Z

@StephanDeHoop , @DanSava : did some refactoring, I will hold off a bit before merging in case you want to have a look.

StephanDeHoop · 2025-01-22T08:45:57Z

@StephanDeHoop , @DanSava : did some refactoring, I will hold off a bit before merging in case you want to have a look.

Looks good, thanks :)!

verveerpj self-assigned this Jan 20, 2025

verveerpj marked this pull request as draft January 20, 2025 10:55

verveerpj force-pushed the new-control-parsing branch 2 times, most recently from 7416b93 to cd0bc4e Compare January 20, 2025 11:13

verveerpj added everest release-notes:refactor PR changes code without changing ANY (!) behavior. labels Jan 20, 2025

verveerpj force-pushed the new-control-parsing branch from cd0bc4e to a2b0a49 Compare January 20, 2025 11:30

verveerpj marked this pull request as ready for review January 20, 2025 11:48

verveerpj force-pushed the new-control-parsing branch from a2b0a49 to 1ffd97d Compare January 21, 2025 14:40

StephanDeHoop approved these changes Jan 21, 2025

View reviewed changes

DanSava reviewed Jan 22, 2025

View reviewed changes

verveerpj added 2 commits January 22, 2025 08:10

Refactor control handling in everest config

a2212ed

Review fixes

4d44cab

verveerpj force-pushed the new-control-parsing branch from 1ffd97d to 4d44cab Compare January 22, 2025 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor control handling in everest config #9805

Refactor control handling in everest config #9805

verveerpj commented Jan 20, 2025

codecov-commenter commented Jan 20, 2025 •

edited

Loading

codspeed-hq bot commented Jan 20, 2025 •

edited

Loading

StephanDeHoop left a comment

StephanDeHoop Jan 21, 2025

verveerpj Jan 22, 2025

StephanDeHoop Jan 21, 2025

verveerpj Jan 22, 2025

StephanDeHoop Jan 21, 2025

verveerpj Jan 22, 2025

StephanDeHoop Jan 22, 2025

StephanDeHoop Jan 21, 2025

DanSava Jan 22, 2025

verveerpj Jan 22, 2025

verveerpj Jan 22, 2025

DanSava Jan 22, 2025 •

edited

Loading

verveerpj Jan 22, 2025

verveerpj commented Jan 22, 2025

StephanDeHoop commented Jan 22, 2025

Refactor control handling in everest config #9805

Are you sure you want to change the base?

Refactor control handling in everest config #9805

Conversation

verveerpj commented Jan 20, 2025

When applicable

codecov-commenter commented Jan 20, 2025 • edited Loading

❌ 1 Tests Failed:

codspeed-hq bot commented Jan 20, 2025 • edited Loading

Merging #9805 will not alter performance

Summary

StephanDeHoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanSava Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verveerpj commented Jan 22, 2025

StephanDeHoop commented Jan 22, 2025

codecov-commenter commented Jan 20, 2025 •

edited

Loading

codspeed-hq bot commented Jan 20, 2025 •

edited

Loading

DanSava Jan 22, 2025 •

edited

Loading