Fix bug where site config was not propagated to Everest config #9719

oyvindeide · 2025-01-13T14:09:41Z

Issue
Resolves #9783

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 logical -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

codspeed-hq · 2025-01-13T14:33:23Z

CodSpeed Performance Report

Merging #9719 will degrade performances by 10.3%

_{Comparing oyvindeide:change_site_config (1017e5d) with main (945e198)}

Summary

❌ 1 regressions
✅ 23 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`oyvindeide:change_site_config`	Change
❌	`test_load_from_context[gen_x: 20, sum_x: 20 reals: 10]`	6.3 ms	7.1 ms	-10.3%

yngve-sk · 2025-01-23T11:17:17Z

src/ert/config/queue_config.py

+                "name",
+                "max_running",
+                "submit_sleep",
+                "qstat_options",


Were these: qstat_options and queue_query_timeout excluded before? If not, should maybe be excluded in a separate commit for clarity

yngve-sk · 2025-01-23T11:33:41Z

src/ert/config/queue_config.py

+        # Use from plugin system if user has not specified
+        plugin_script = None
+        if info.context:
+            plugin_script = info.context.get(info.field_name)


Is there a test that runs this line? EDIT: I see running test_detached triggers it, but do any of the ERT tests run it? Is this mainly meant for things we run via Everest?

DanSava

Looks very good, requires a rebase.

DanSava · 2025-01-23T09:31:07Z

src/ert/config/queue_config.py

@@ -120,17 +130,13 @@ class LsfQueueOptions(QueueOptions):

    @property
    def driver_options(self) -> dict[str, Any]:
-        driver_dict = asdict(self)
-        driver_dict.pop("name")
+        driver_dict = self.model_dump(exclude={"name", "submit_sleep", "max_running"})


Very nice ❤️

DanSava · 2025-01-23T11:14:56Z

src/everest/config/everest_config.py

+        if site_config:
+            context["queue_system"] = QueueConfig.from_dict(site_config).queue_options
+        if activate_script:
+            context["activate_script"] = ErtPluginManager().activate_script()


I think you can just have

context["activate_script"] = activate_script

in place of

context["activate_script"] = ErtPluginManager().activate_script()

DanSava · 2025-01-23T11:30:53Z

src/everest/config/everest_config.py

@@ -276,7 +268,8 @@ def validate_forward_model_job_name_installed(self) -> Self:  # pylint: disable=
            return self
        installed_jobs_name = [job.name for job in install_jobs]
        installed_jobs_name += list(script_names)  # default jobs
-        installed_jobs_name += get_system_installed_jobs()  # system jobs
+        if info.context:  # Add plugin jobs
+            installed_jobs_name += info.context.get("install_jobs", {}).keys()


It might have been possible to just move the logic in the function get_system_installed_jobs and just remove the _dummy_ert_config function

site_config = ErtConfig.read_site_config() ert_config: ErtConfig = ErtConfig.with_plugins().from_dict( config_dict=site_config ) context = { "install_jobs": ert_config.installed_forward_model_steps, }

and avoid setting this through the context, but I guess this works as well.

DanSava · 2025-01-23T11:33:07Z

src/everest/config/everest_config.py

-    @staticmethod
-    def load_file(config_file: str) -> "EverestConfig":
+    @classmethod
+    def load_file(cls, config_file: str):


Shouldn't this still have a return type? Maybe Self?

DanSava · 2025-01-23T11:35:37Z

src/everest/config/everest_config.py

@@ -807,6 +800,23 @@ def load_file(config_file: str) -> "EverestConfig":

            raise exp from error

+    @classmethod
+    def with_plugins(cls, config_dict):


I am a bit confused as to why mypy is not complaining about the missing type for the argument config_dict shouldn't there be a : dict[str, Any] or something similar?

DanSava · 2025-01-23T11:36:51Z

tests/everest/test_detached.py

-
-def test_detached_mode_config_base(copy_math_func_test_data_to_tmp):
-    everest_config, _ = _get_reference_config()
+def test_detached_mode_config_base(min_config, monkeypatch, tmp_path):


Nice cleanup ❤️

yngve-sk · 2025-01-23T11:39:57Z

src/everest/config/everest_config.py

@@ -814,6 +815,9 @@ def load_file(config_file: str) -> "EverestConfig":
    def with_plugins(cls, config_dict):
        context = {}
        activate_script = ErtPluginManager().activate_script()
+        site_config = ErtConfig.read_site_config()
+        if site_config:
+            context["queue_system"] = QueueConfig.from_dict(site_config).queue_options
        if activate_script:
            context["activate_script"] = ErtPluginManager().activate_script()


Is there a need to re-invoke ErtPluginManager().activate_script() after having stored it in a variable?

yngve-sk · 2025-01-23T11:51:33Z

src/everest/config/simulator_config.py

-            return LocalQueueOptions(max_running=8)
+            options = None
+            if info.context:
+                options = info.context.get(info.field_name)


Won't info.field_name always be queue_system here? Similar question for other instances of this

oyvindeide force-pushed the change_site_config branch 8 times, most recently from da1f1db to 1d74782 Compare January 16, 2025 08:27

oyvindeide self-assigned this Jan 16, 2025

oyvindeide added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Jan 16, 2025

oyvindeide force-pushed the change_site_config branch 2 times, most recently from 116ec8f to f9b110d Compare January 17, 2025 08:31

oyvindeide changed the title ~~Add function to propagate context to validation~~ Fix bug where site config was not propagated to Everest config Jan 17, 2025

oyvindeide force-pushed the change_site_config branch 5 times, most recently from 1ae7af5 to e8f8737 Compare January 17, 2025 11:15

oyvindeide added 5 commits January 17, 2025 14:26

Convert from dataclass to BaseModel

f5b6de1

Add function to propagate context to validation

6337e95

Fix bug where queue settings were not taken from site config

584a340

Add installed jobs in new context system

7bd65e4

Simplify test

1017e5d

oyvindeide force-pushed the change_site_config branch from e8f8737 to 1017e5d Compare January 17, 2025 13:27

yngve-sk reviewed Jan 23, 2025

View reviewed changes

DanSava reviewed Jan 23, 2025

View reviewed changes

yngve-sk reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where site config was not propagated to Everest config #9719

Fix bug where site config was not propagated to Everest config #9719

oyvindeide commented Jan 13, 2025 •

edited

Loading

codspeed-hq bot commented Jan 13, 2025 •

edited

Loading

yngve-sk Jan 23, 2025

yngve-sk Jan 23, 2025 •

edited

Loading

DanSava left a comment

DanSava Jan 23, 2025

DanSava Jan 23, 2025

DanSava Jan 23, 2025

DanSava Jan 23, 2025

DanSava Jan 23, 2025

DanSava Jan 23, 2025

yngve-sk Jan 23, 2025 •

edited

Loading

yngve-sk Jan 23, 2025

Fix bug where site config was not propagated to Everest config #9719

Are you sure you want to change the base?

Fix bug where site config was not propagated to Everest config #9719

Conversation

oyvindeide commented Jan 13, 2025 • edited Loading

When applicable

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

Merging #9719 will degrade performances by 10.3%

Summary

Benchmarks breakdown

Choose a reason for hiding this comment

yngve-sk Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

DanSava 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

yngve-sk Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyvindeide commented Jan 13, 2025 •

edited

Loading

codspeed-hq bot commented Jan 13, 2025 •

edited

Loading

yngve-sk Jan 23, 2025 •

edited

Loading

yngve-sk Jan 23, 2025 •

edited

Loading