-
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
Print overwritten Queue Options values to log files #6037
Print overwritten Queue Options values to log files #6037
Conversation
src/ert/config/queue_config.py
Outdated
selected_queue_system: QueueSystem, queue_system_options: List[Tuple[str, str]] | ||
) -> None: | ||
temp_dict: Dict[str, List[str]] = defaultdict(list) | ||
for option_string 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.
why are we checking if instance is tuple if we are expecting List[Tuple[str, str]]
If the type hint is not what we expect, then we either explicitly hint unstructured data type or data needs to sturctured before referencing this function.
then the function can be simple
for option_name, option_value in queue_system_options:
temp_dict.setdefault(option_name, []).append(option_value)
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 to modify and complicate this part because I discovered that the option_value might be left unspecified in the config file in which case there is only one value to unpack.
src/ert/config/queue_config.py
Outdated
option_name = option_string[0] | ||
option_value = option_string[1] | ||
temp_dict.setdefault(option_name, []).append(option_value) | ||
for option_name, option_values in temp_dict.items(): |
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.
if we split the function in two, the first and second for loop. then we can make the first into a generator. because the actual behaviour of this function is done in the second part. the first seems to just "structurize" the data
src/ert/config/queue_config.py
Outdated
and queue_options[selected_queue_system] | ||
): | ||
_check_for_overwritten_queue_system_options( | ||
selected_queue_system, queue_options[selected_queue_system] |
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 to modify and complicate this part because I discovered that the option_value might be left unspecified in the config file in which case there is only one value to unpack.
If that is the case, try passing a filter or filter generator to the function.
example
(options_string for opitons_string in queue_options[selected_queue_system] if isinstance(option_string, tuple))
This allows you to loop once through the data set instead of twice.
This is a matter of opinion but it seems expensive doing two consecutive loops on the same data set for a log.
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.
plus look into Counter
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.
LGTM
fd64438
to
ad6a535
Compare
c8066e9
to
eb23a82
Compare
Issue
Resolves #5715
Approach
Short description of the approach
Pre review checklist
Pre merge checklist
guidelines.