HWS: Extra error checking and some style cleanup #140
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This has 2 advantages: 1) Performance: fewer syscalls due to no longer repeatedly closing and reopening the same file 2) Exception safety, guaranteeing that this file will be closed at the end of the `with` clause Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
We do not write to this file, so there is no reason to open it in `r+` (read + update) mode. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
No need to convert it to a string - convert it only where necessary. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
Now that we always open() the file path directly, we don't need to keep a reference to the file descriptor. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
5d58d63 to
b03eb5d
Compare
Replace "== None" with "is None". According to PEP8, this is the correct way to compare to singletons like None. No behavioural changes. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
Equality comparisons with boolean literals are unnecessary and are considered bad style as per PEP8. Replace them with the more pythonic implicit conversion to boolean in if statements. No behavioural changes. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
If the CSV file is malformed, the parser will print an error message and skip the malformed line rather than crashing. Also add type annotations to the parsing function. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
Stop using a global variable for the unsupported object list. Instead, return and pass the value as a local variable into the function. No behavioural changes - the printed output is exactly the same. If changing the output format is fine, then the join call can be removed. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
The script now checks the Python version and exits if it's lower than the minimum supported version. The alternative is cryptic stack traces that users may not understand - it is inevitable that we use *some* features that are not available in older versions of Python. The printed error looks like this: ``` > python38 hws/mlx_hw_steering_parser.py Error: mlx_hw_steering_parser.py requires Python 3.9 or newer ``` Before this changeset, the minimum supported version was at least 3.6 due to using f-strings. We can validate this by running `pipx run vermin -vvv hws/mlx_hw_steering_parser.py`. With this change, we set the minimum to 3.9. This should be reasonable as 3.9 is currently the oldest supported version. It was released on 2020-10-05 and will be EOL on 2025-10. The DOCA dev container is based on Ubuntu 22.04, which has Python 3.10. Signed-off-by: Alexander Efimov <aefimov@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean up various things in the code base for the HWS dump tool. I'd like to get these things in before creating the PR for JSON output to keep the PRs more or less isolated.
This PR depends on #110 being merged first - this branch is based on that one. It's fine to also merge this one as is and discard the other one.
Note the last commit in this series - it bumps the minimum supported Python version from 3.6 to 3.9. If that's a problem, please let me know the minimum version we need to support.