Fix general queue options not being added bug #8619
Conversation
jonathan-eq
added
the
release-notes:bug-fix
|
The bug is occurring for the keywords max_running, submit_sleep, and project code. |
jonathan-eq
force-pushed
the
fix-bug
branch
5 times, most recently
from
28bb096 to
708cf2b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8619 +/- ##
==========================================
+ Coverage 90.85% 90.91% +0.05%
==========================================
Files 339 343 +4
Lines 20852 21289 +437
==========================================
+ Hits 18946 19355 +409
- Misses 1906 1934 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
jonathan-eq
force-pushed
the
fix-bug
branch
2 times, most recently
from
162c109 to
11ffe8f
Compare
Unsure if project code is affected |
jonathan-eq
force-pushed
the
fix-bug
branch
2 times, most recently
from
3fac2eb to
5df708f
Compare
src/ert/config/ert_config.py
Outdated
| filtered_queue_options = [] | ||
| for queue_option in value: | ||
| if ( | ||
| "MAX_RUNNING" in queue_option |
There was a problem hiding this comment.
This adds a maintenance problem when we add more queue options later. Can this be automated so we don't need to specifically test for MAX_RUNNING and SUBMIT_SLEEP here?
There was a problem hiding this comment.
I am not sure, as those are the only options that can be both global and queue specific.
There was a problem hiding this comment.
I can test if the string is outside of QUEUE_OPTION, but I wonder if this can cause other bugs which will be difficult to troubleshoot.
| if keyword == "QUEUE_OPTION": | ||
| filtered_queue_options = [] | ||
| for queue_option in value: | ||
| option_name = queue_option[1] |
There was a problem hiding this comment.
I am not a fan of this. Maybe we can change QUEUE_OPTIONS to be Dict[str, Dict[str, str] instead of List[List[str]]?
Something like { "LSF": {"bsub_cmd": "/path/to/cmd"} }
We do this somewhere later on, maybe it can be moved up here instead?
I assume this is due to legacy reasons @berland
There was a problem hiding this comment.
It seems this would require a larger refactoring of ConfigDict, which will not be of priority right now 🌧️
There was a problem hiding this comment.
They are List[List[str]] initially so we can reason about the order of settings before we interpret them into a Dict.
| ), | ||
| ], | ||
| ) | ||
| def test_queue_config_general_max_running_takes_precedence_over_queue_option( |
There was a problem hiding this comment.
Hm, is it really so? I thought that the queue_option takes precedence ... always
jonathan-eq
force-pushed
the
fix-bug
branch
from
3aa9b3a to
33d0c2d
Compare
|
Looks very good @jonathan-eq ! Could you squash the commits and improve the commit message? |
This commit fixes the issue where the general options `MAX_RUNNING` and `SUBMIT_SLEEP` would have priority over the equivalent QUEUE_OPTIONS. It also makes the options set in user config for those keywords take precedence over what is set in site config.
jonathan-eq
force-pushed
the
fix-bug
branch
from
33d0c2d to
5f58168
Compare
Issue
Resolves #8605
Approach
🔧
(Screenshot of new behavior in GUI if applicable)
When applicable