-
Notifications
You must be signed in to change notification settings - Fork 107
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 queue option validation to queue config #6413
Conversation
b0376a6
to
2601536
Compare
6a336e9
to
865f233
Compare
If you expose the |
97a7662
to
1ff6e86
Compare
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not assert that the string MAX_RUNNING is part of the error? (via regexp if needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, tested further below. Do we need this test here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, maybe we can skip it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below lacks testing of "-1". The happy path ("100") is probably tested within the hypothesis setup I suppose.
c7c9ec0
to
658b66e
Compare
Noting that this PR will allow f.ex. |
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 Report
@@ 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
... 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! |
Should be possible to issue |
e0658bd
to
33da07f
Compare
src/ert/config/queue_config.py
Outdated
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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I think it must to be possible to have ert config files that can be used both on LSF and Torque without warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we raise warnings if different queue is set wrongly.
6f4f3de
to
39a2783
Compare
src/ert/config/queue_config.py
Outdated
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe configured_queue_systems
is more descriptive. I had to read the source in order to understand what used
meant in this context.
src/ert/config/queue_config.py
Outdated
): | ||
_validate_torque_options(queue_options[QueueSystem.TORQUE]) | ||
|
||
for queue_system_val in used_queue_systems: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead of used_queue_systems
do something like queue_options.keys()
?
src/ert/config/queue_config.py
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more consistent to use QueueSystem.LSF
instead of the string LSF
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
39a2783
to
427d419
Compare
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.
427d419
to
5aa7ee6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go!
Issue
Resolves #6391
Approach
Add validation of option types to each driver.
test_that_queue_config_dict_negative_value_invalid
testPre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist