Fix a type mismatch in - #366
Conversation
gitkenan
left a comment
There was a problem hiding this comment.
Two small comments about the approach here, but overall looks great
| normalised: WorkloadType = {} | ||
| for key, value in options.items(): | ||
| if isinstance(value, list): | ||
| normalised[key] = [f"{item}" for item in value] |
There was a problem hiding this comment.
In Python:
bool("False") # is True! Non-empty strings are always truthy.
Bob's recommendation: Either preserve bool types, convert booleans properly, or normalize boolean string handling in _parse_options() (e.g. str(value).lower() in ("true", "1")). Unconditionally formatting bool to f"{value}" breaks boolean semantics.
| """ | ||
| normalised: WorkloadType = {} | ||
| for key, value in options.items(): | ||
| if isinstance(value, list): |
There was a problem hiding this comment.
This only handles 1 level of nesting (list vs non-list).
If an option is a nested dict (e.g., auth: {config: ..., s3_session_token: ...} in elbencho or prefill: {blocksize: '4M', numjobs: 1}), f"{value}" stringifies the entire dictionary into "{'blocksize': '4M', 'numjobs': 1}", which could corrupt nested configuration structures
There was a problem hiding this comment.
By the time the code gets here there should be no nested options. prefill is not included in the workloads section, it is handled by the separate prefill() method, which is called at line 75 in cbt.py. Prefill should be a property of the benchmark, not the Workload. We could change this in the future, but then that change would also have to consider the changes required here to handle the new format correctly.
The type to be passed is a dict[str, WorkloadType] where WorkloadType is dict[str, Union[str, list[str]]]
The elbencho code needs to honour this type when calling _create_configurations()
As part of a code review for PR 359 it was noticed that in some instances the command classes were being initalised with a dict containing str, int, bool values when the constructor was explicitly expecting strings. Fix that type mis-match so that the options passed in to the *command classes are definitely in a dict[str, Union[str, list[str]] format IBM Bob 2.0.3 was used to help with this code. Signed-off-by: Chris Harris <harriscr@uk.ibm.com>
6705275 to
950ffeb
Compare
As part of a code review for #359 it was noticed that in some instances the command classes were being initalised with a dict containing str, int, bool values when the constructor was explicitly expecting strings. Fix that type mis-match so that the options passed in to the *command classes are definitely in a dict[str, Union[str, list[str]] format.
The code worked as written, but the type mismatch could lead to unexpected behaviour
IBM Bob 2.0.3 was used to help with this code.
###Testing###
New unit tests were written to catch this case.
All unit tests pass:
Manual testing shows the code working as expected after this change