Skip to content
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

Merged

Conversation

larsevj
Copy link
Contributor

@larsevj larsevj commented Sep 4, 2023

Issue
Resolves #5715

Approach
Short description of the approach

Pre review checklist

  • Read through the code changes carefully after finishing work
  • Make sure tests pass locally (after every commit!)
  • Wait for tests in CI to pass (see "checks" below)
  • 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 new behaviour is tested (opened GUI? screenshots? tried workflows?)
  • Commit history is consistent and clean, in line with the contribution guidelines.

Pre merge checklist

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

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:
Copy link
Contributor

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)

Copy link
Contributor Author

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.

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():
Copy link
Contributor

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

and queue_options[selected_queue_system]
):
_check_for_overwritten_queue_system_options(
selected_queue_system, queue_options[selected_queue_system]
Copy link
Contributor

@sregales-TNO sregales-TNO Sep 6, 2023

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus look into Counter

Copy link
Contributor

@sregales-TNO sregales-TNO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@larsevj larsevj force-pushed the logging_when_overwriting_queue_options branch from fd64438 to ad6a535 Compare September 18, 2023 10:31
@larsevj larsevj added release-notes:misc Automatically categorise as miscellaneous change in release notes release-notes:skip If there should be no mention of this in release notes and removed release-notes:misc Automatically categorise as miscellaneous change in release notes labels Sep 18, 2023
@larsevj larsevj force-pushed the logging_when_overwriting_queue_options branch from c8066e9 to eb23a82 Compare September 18, 2023 11:08
@larsevj larsevj merged commit d5ea887 into equinor:main Sep 18, 2023
40 of 42 checks passed
@larsevj larsevj deleted the logging_when_overwriting_queue_options branch September 18, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:skip If there should be no mention of this in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

LSF_SERVER config linting
2 participants