Add queue option validation to queue config #6413

xjules · 2023-10-24T11:45:29Z

Issue
Resolves #6391

Approach
Add validation of option types to each driver.

Remove test_that_queue_config_dict_negative_value_invalid test

Pre review checklist

Read through the code changes carefully after finishing work
Make sure tests pass locally (after every commit!)
Prepare changes in small commits for more convenient review (optional)
PR title captures the intent of the changes, and is fitting for release notes.
Updated documentation
Ensured that unit tests are added for all new behavior (See
Ground Rules),
and changes to existing code have good test coverage.

Pre merge checklist

Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.

src/ert/config/queue_config.py

tests/unit_tests/config/test_ert_config.py

berland · 2023-10-27T08:20:29Z

If you expose the string_options++ in queue_config.py to tests/unit_tests/config/config_dict_generator.py I think you can make the tests quite smart.

src/ert/config/queue_config.py

src/ert/job_queue/driver.py

berland · 2023-11-01T12:48:19Z

tests/unit_tests/config/test_ert_config.py

@@ -707,8 +707,8 @@ def test_default_ens_path(tmpdir):
    "max_running_value, expected_error",
    [
        (100, None),  # positive integer
-        (-1, "QUEUE_OPTION MAX_RUNNING is negative"),  # negative integer
-        ("not_an_integer", "QUEUE_OPTION MAX_RUNNING is not an integer"),  # non-integer
+        (-1, "is not a valid positive integer"),  # negative integer


Should we not assert that the string MAX_RUNNING is part of the error? (via regexp if needed)

ah, tested further below. Do we need this test here then?

Not sure, maybe we can skip it?

The code below lacks testing of "-1". The happy path ("100") is probably tested within the hypothesis setup I suppose.

berland · 2023-11-01T14:26:39Z

Noting that this PR will allow f.ex. QUEUE_OPTION LOCAL MAX_RUNNING fooo if the queue system is set to Torque. Ert-main will not allow fooo here.

xjules · 2023-11-02T12:33:32Z

Noting that this PR will allow f.ex. QUEUE_OPTION LOCAL MAX_RUNNING fooo if the queue system is set to Torque. Ert-main will not allow fooo here.

I see that as up discussion whether we allow some general configs for instance related to other queues (LOCAL) to be set when using TORQUE for instance.

codecov-commenter · 2023-11-02T12:40:06Z

Codecov Report

Merging #6413 (5aa7ee6) into main (15c4ae4) will increase coverage by 0.01%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main    #6413      +/-   ##
==========================================
+ Coverage   83.47%   83.48%   +0.01%     
==========================================
  Files         341      341              
  Lines       20709    20717       +8     
  Branches      938      938              
==========================================
+ Hits        17287    17296       +9     
+ Misses       3131     3130       -1     
  Partials      291      291

Files	Coverage Δ
src/ert/config/__init__.py	`100.00% <100.00%> (ø)`
src/ert/job_queue/driver.py	`97.72% <66.66%> (-2.28%)`	⬇️
src/ert/config/queue_config.py	`97.43% <93.10%> (-2.57%)`	⬇️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

berland · 2023-11-02T12:55:24Z

I see that as up discussion whether we allow some general configs for instance related to other queues (LOCAL) to be set when using TORQUE for instance.

Should be possible to issue ConfigWarning for errors in not-selected queues.

berland · 2023-11-03T12:15:22Z

src/ert/config/queue_config.py

-                        f"Setting '{option_name}'for {queue_system.name} but "
-                        f"{selected_queue_system.name} is selected. ",
+                        f"Setting '{option_name}' for {queue_system.name} but "
+                        f"{selected_queue_system.name} is selected.",


Will this give warnings if I have valid Torque queue options but Local queue is selected? I don't think I want that, I only want warnings if I have invalid Torque queue options and local is selected.

I think we should be validating only when the queue is chosen otherwise just give a warning that you are setting something that is not in used anyway.

Nah, I think it must to be possible to have ert config files that can be used both on LSF and Torque without warnings.

Now we raise warnings if different queue is set wrongly.

berland · 2023-11-06T13:59:26Z

src/ert/config/queue_config.py

@@ -77,13 +52,16 @@ def from_dict(cls, config_dict: ConfigDict) -> QueueConfig:
        job_script = job_script or "job_dispatch.py"
        max_submit: int = config_dict.get("MAX_SUBMIT", 2)
        queue_options: Dict[QueueSystem, List[Tuple[str, str]]] = defaultdict(list)
+        used_queue_systems: Set[QueueSystem] = set()


maybe configured_queue_systems is more descriptive. I had to read the source in order to understand what used meant in this context.

berland · 2023-11-06T14:02:02Z

src/ert/config/queue_config.py

-        ):
-            _validate_torque_options(queue_options[QueueSystem.TORQUE])
+
+        for queue_system_val in used_queue_systems:


Can we instead of used_queue_systems do something like queue_options.keys()?

berland · 2023-11-06T14:05:25Z

src/ert/config/queue_config.py

+def _validate_queue_driver_settings(
+    queue_system_options: List[Tuple[str, str]], queue_type: str, throw_error: bool
+) -> None:
+    for option_strings in queue_system_options:


We can do unpacking of the tuple on the for line, for option_name, option_value in queue_system_options: and then remove the two following lines.

berland · 2023-11-06T14:07:58Z

src/ert/config/queue_config.py

-def _validate_torque_options(torque_options: List[Tuple[str, str]]) -> None:
-    for option_strings in torque_options:
+queue_memory_options: Mapping[str, List[str]] = {
+    "LSF": [],


Is it more consistent to use QueueSystem.LSF instead of the string LSF?

I had that before, but I changed it due to these *_options are now exposed in tests and didn't want to import all the Cwrap classes there.

This still supports unsetting the options when providing an empty string. Additionally, it raises ValueError when we are not able to set the option when starting simulation. If different queue system is ajusted, than currently in use, we validate and issue only warning instead. We expose queue_bool_options, queue_memory_options, queue_positive_int_options, queue_positive_number_options, queue_string_options and queue_memory_options from ert.config. NB: when DEBUG_OUTPUT is deprecated it needs to be removed from queue_options.

berland

Lets go!

xjules added the release-notes:misc Automatically categorise as miscellaneous change in release notes label Oct 24, 2023

xjules self-assigned this Oct 24, 2023

xjules force-pushed the check_queue_option_types branch from b0376a6 to 2601536 Compare October 25, 2023 11:01

xjules marked this pull request as ready for review October 25, 2023 11:02

xjules force-pushed the check_queue_option_types branch 4 times, most recently from 6a336e9 to 865f233 Compare October 26, 2023 13:25

berland requested review from berland and larsevj October 27, 2023 08:01

berland reviewed Oct 27, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Oct 27, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Oct 27, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Oct 27, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Oct 27, 2023

View reviewed changes

tests/unit_tests/config/test_ert_config.py Outdated Show resolved Hide resolved

larsevj requested changes Oct 27, 2023

View reviewed changes

src/ert/config/queue_config.py Show resolved Hide resolved

src/ert/config/queue_config.py Show resolved Hide resolved

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

xjules force-pushed the check_queue_option_types branch 2 times, most recently from 97a7662 to 1ff6e86 Compare October 31, 2023 09:38

berland reviewed Nov 1, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Nov 1, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Nov 1, 2023

View reviewed changes

src/ert/config/queue_config.py Outdated Show resolved Hide resolved

berland reviewed Nov 1, 2023

View reviewed changes

src/ert/job_queue/driver.py Show resolved Hide resolved

berland reviewed Nov 1, 2023

View reviewed changes

xjules force-pushed the check_queue_option_types branch 2 times, most recently from c7c9ec0 to 658b66e Compare November 1, 2023 13:21

xjules force-pushed the check_queue_option_types branch from e0658bd to 33da07f Compare November 2, 2023 13:58

berland reviewed Nov 3, 2023

View reviewed changes

xjules force-pushed the check_queue_option_types branch 2 times, most recently from 6f4f3de to 39a2783 Compare November 6, 2023 13:49

berland requested changes Nov 6, 2023

View reviewed changes

xjules force-pushed the check_queue_option_types branch from 39a2783 to 427d419 Compare November 7, 2023 11:07

xjules force-pushed the check_queue_option_types branch from 427d419 to 5aa7ee6 Compare November 7, 2023 11:24

berland approved these changes Nov 7, 2023

View reviewed changes

xjules enabled auto-merge (rebase) November 7, 2023 11:42

larsevj approved these changes Nov 7, 2023

View reviewed changes

xjules merged commit 9bea497 into equinor:main Nov 7, 2023
43 of 44 checks passed

xjules deleted the check_queue_option_types branch November 9, 2023 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add queue option validation to queue config #6413

Add queue option validation to queue config #6413

xjules commented Oct 24, 2023 •

edited

Loading

berland commented Oct 27, 2023 •

edited

Loading

berland Nov 1, 2023

berland Nov 1, 2023

xjules Nov 1, 2023

berland Nov 2, 2023

berland commented Nov 1, 2023

xjules commented Nov 2, 2023

codecov-commenter commented Nov 2, 2023 •

edited

Loading

berland commented Nov 2, 2023

berland Nov 3, 2023

xjules Nov 3, 2023 •

edited

Loading

berland Nov 3, 2023

xjules Nov 6, 2023

berland Nov 6, 2023

berland Nov 6, 2023

berland Nov 6, 2023

berland Nov 6, 2023

xjules Nov 7, 2023 •

edited

Loading

berland Nov 7, 2023

berland left a comment

Add queue option validation to queue config #6413

Add queue option validation to queue config #6413

Conversation

xjules commented Oct 24, 2023 • edited Loading

Pre review checklist

Pre merge checklist

berland commented Oct 27, 2023 • edited Loading

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

berland commented Nov 1, 2023

xjules commented Nov 2, 2023

codecov-commenter commented Nov 2, 2023 • edited Loading

Codecov Report

berland commented Nov 2, 2023

Choose a reason for hiding this comment

xjules Nov 3, 2023 • edited Loading

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

xjules Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berland left a comment

Choose a reason for hiding this comment

xjules commented Oct 24, 2023 •

edited

Loading

berland commented Oct 27, 2023 •

edited

Loading

codecov-commenter commented Nov 2, 2023 •

edited

Loading

xjules Nov 3, 2023 •

edited

Loading

xjules Nov 7, 2023 •

edited

Loading