[ISSUE-41]: Refactor cpu_utils.py#50
Open
lmzuccarelli wants to merge 4 commits intoredhat-et:mainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a frozen Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 54-61: Inside __init__ remove the dead local property and unused
attribute: delete the local definition of `@property` cpu_entries and the
self.__cpu_entries assignment, and also remove the commented-out _node_to_cpus
and _node_core_to_min_cpu lines; if a public read-only accessor is required, add
a class-level property named cpu_entries that returns tuple(self._cpu_entries)
so callers use the existing _cpu_entries list; update references to use
_cpu_entries and ensure CpuInfo typing remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4349dd62-c603-435c-92da-b35921829289
📒 Files selected for processing (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py (1)
387-397:⚠️ Potential issue | 🟡 MinorFix trailing whitespace to pass pre-commit hook.
The pipeline is failing due to trailing whitespace detected by the pre-commit hook. Ensure no trailing whitespace exists at the end of lines (likely line 397).
🔧 Proposed fix
def filters(self): return { 'cpu_list_to_range': cpu_list_to_range, 'extract_primary_cpus': extract_primary_cpus, 'extract_all_cpus': extract_all_cpus, 'extract_numa_nodes': extract_numa_nodes, 'merge_cpu_ranges': merge_cpu_ranges, 'extract_size_value': extract_size_value, - } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 387 - 397, Remove trailing whitespace in the FilterModule definition (the filters method returning the dict of filter names such as cpu_list_to_range, extract_primary_cpus, etc.). Edit the end of that return block to strip any spaces at the ends of lines (especially the final line after the dict) and ensure the file ends with a single newline with no trailing spaces so the pre-commit hook passes.
♻️ Duplicate comments (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py (1)
54-62:⚠️ Potential issue | 🟠 MajorRemove dead code: property defined inside
__init__is invalid Python.Lines 54-57 define a
@propertydecorator and method inside__init__, which creates a local function that is immediately discarded—not a class-level property. The__cpu_entriesattribute (line 54) is also never used; all actual logic uses_cpu_entries(line 59).🔧 Proposed fix
if not isinstance(lscpu_data, str): raise LscpuParseError( f"Expected string input, got {type(lscpu_data).__name__}" ) - self.__cpu_entries: List[CpuInfo] = [] - `@property` - def cpu_entries(self) -> Sequence[CpuInfo]: - return tuple(self.__cpu_entries) - self._cpu_entries: List[CpuInfo] = [] self._numa_nodes: Set[int] = set()If a public read-only accessor is needed, define it at the class level:
`@property` def cpu_entries(self) -> Sequence[CpuInfo]: return tuple(self._cpu_entries)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 54 - 62, Remove the dead local property and unused private attribute created inside __init__: delete the __cpu_entries definition and the `@property` cpu_entries method that currently lives inside __init__; instead, add a class-level read-only accessor named cpu_entries that returns tuple(self._cpu_entries) so callers get an immutable Sequence[CpuInfo], and keep using the existing _cpu_entries storage and other structures (_numa_nodes, _node_to_cpus, _node_core_to_min_cpu) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 387-397: Remove trailing whitespace in the FilterModule definition
(the filters method returning the dict of filter names such as
cpu_list_to_range, extract_primary_cpus, etc.). Edit the end of that return
block to strip any spaces at the ends of lines (especially the final line after
the dict) and ensure the file ends with a single newline with no trailing spaces
so the pre-commit hook passes.
---
Duplicate comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 54-62: Remove the dead local property and unused private attribute
created inside __init__: delete the __cpu_entries definition and the `@property`
cpu_entries method that currently lives inside __init__; instead, add a
class-level read-only accessor named cpu_entries that returns
tuple(self._cpu_entries) so callers get an immutable Sequence[CpuInfo], and keep
using the existing _cpu_entries storage and other structures (_numa_nodes,
_node_to_cpus, _node_core_to_min_cpu) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0e01adf-12ed-4f90-a4a6-005af846e07a
📒 Files selected for processing (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 220-221: The guards that return empty values when lscpu_data is
missing or not a string should instead raise AnsibleFilterError so callers fail
fast; locate the checks that examine lscpu_data (the if not lscpu_data or not
isinstance(lscpu_data, str) conditions) and replace the silent return with
raising AnsibleFilterError including a clear message like "expected lscpu output
string, got: <type/value>"; apply the same change to the other similar guards
around the function(s) handling topology parsing (the checks at the other
occurrences reported at lines 249-250 and 277-278) so parsing errors surface
immediately.
- Around line 302-304: The loop currently silences non-string elements by
continuing; change it to only skip empty strings and raise AnsibleFilterError
for unexpected types: inside the for idx, cpu_range in enumerate(range_list)
block, if cpu_range == "" (or cpu_range.strip() == ""), continue; if not
isinstance(cpu_range, str), raise AnsibleFilterError with a message that
includes idx and the offending type/value so callers can debug; ensure
AnsibleFilterError is imported/available in cpu_utils.py and use the identifiers
range_list, cpu_range and idx in the error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ece7acac-3b15-4117-90de-1918cbb164af
📒 Files selected for processing (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py (1)
92-98: Redundant check due todefaultdict.Since
_node_core_to_min_cpuis initialized asdefaultdict(dict)at line 57, the explicit check on lines 93-94 is unnecessary—accessing a missing key automatically creates an empty dict.♻️ Suggested simplification
- # Index by NUMA node and core for primary CPU tracking - if node not in self._node_core_to_min_cpu: - self._node_core_to_min_cpu[node] = {} - - core_dict = self._node_core_to_min_cpu[node] + # Index by NUMA node and core for primary CPU tracking + core_dict = self._node_core_to_min_cpu[node]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 92 - 98, The explicit existence check for node before assigning self._node_core_to_min_cpu[node] is redundant because _node_core_to_min_cpu is a defaultdict(dict); simplify the block by directly obtaining core_dict = self._node_core_to_min_cpu[node] without the if node not in ... check, then keep the existing min-selection logic (if core not in core_dict or cpu < core_dict[core]: core_dict[core] = cpu) so primary CPU tracking behavior remains unchanged; update the code around the assignment to remove the unnecessary branch referencing _node_core_to_min_cpu, node, core_dict, core, and cpu.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Line 89: There is a trailing whitespace in the cpu_utils.py module that trips
the pre-commit hook; open the cpu_utils.py file and remove any trailing spaces
on the blank/empty line around the reported area (near the top of the file
around the existing cpu utility functions), then run `pre-commit run
--all-files` to ensure the trailing-whitespace hook passes before committing.
---
Nitpick comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 92-98: The explicit existence check for node before assigning
self._node_core_to_min_cpu[node] is redundant because _node_core_to_min_cpu is a
defaultdict(dict); simplify the block by directly obtaining core_dict =
self._node_core_to_min_cpu[node] without the if node not in ... check, then keep
the existing min-selection logic (if core not in core_dict or cpu <
core_dict[core]: core_dict[core] = cpu) so primary CPU tracking behavior remains
unchanged; update the code around the assignment to remove the unnecessary
branch referencing _node_core_to_min_cpu, node, core_dict, core, and cpu.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61419423-987d-478b-886f-04f28b989435
📒 Files selected for processing (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py
There was a problem hiding this comment.
♻️ Duplicate comments (3)
automation/test-execution/ansible/filter_plugins/cpu_utils.py (3)
302-304:⚠️ Potential issue | 🟠 MajorNon-string elements in
range_listare silently dropped.If a templating bug passes
Noneor an integer, the merged affinity mask becomes incomplete with no error. Empty strings should be skipped, but unexpected types should raise.🔧 Proposed fix
for idx, cpu_range in enumerate(range_list): - if not cpu_range or not isinstance(cpu_range, str): - continue + if not isinstance(cpu_range, str): + raise AnsibleFilterError( + f"Invalid element at index {idx}: expected string, got {type(cpu_range).__name__}" + ) + if not cpu_range.strip(): + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 302 - 304, The loop over range_list silently skips non-string values (cpu_range) which masks templating bugs; update the iteration (the for idx, cpu_range in enumerate(range_list) block) to continue only for empty strings but raise a TypeError for any cpu_range that is not a str, including the offending index and value in the error message so callers can diagnose the bad input rather than producing an incomplete affinity mask.
249-250:⚠️ Potential issue | 🟠 MajorInconsistent error handling:
extract_all_cpussilently returns empty whileextract_primary_cpusraises.
extract_primary_cpus(lines 218-221) raisesAnsibleFilterErrorfor invalid input, butextract_all_cpussilently returns"". Since both consumelscpuoutput from the same source (per the playbook), this should fail fast consistently.🔧 Proposed fix
if not lscpu_data or not isinstance(lscpu_data, str): - return "" + raise AnsibleFilterError( + f"Invalid lscpu input: expected non-empty string, got {type(lscpu_data).__name__}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 249 - 250, The function extract_all_cpus currently returns an empty string on invalid input while extract_primary_cpus raises AnsibleFilterError; update extract_all_cpus to mirror extract_primary_cpus by raising AnsibleFilterError when lscpu_data is falsy or not a str instead of returning "", i.e., replace the early-return in extract_all_cpus with a raise AnsibleFilterError carrying a clear message about invalid lscpu input so both functions fail fast and behave consistently.
277-278:⚠️ Potential issue | 🟠 MajorSame inconsistency:
extract_numa_nodesshould fail fast on invalid input.Similar to
extract_all_cpus, this silently returns[]for invalid input whileextract_primary_cpusraises. Apply the same fix for consistency.🔧 Proposed fix
if not lscpu_data or not isinstance(lscpu_data, str): - return [] + raise AnsibleFilterError( + f"Invalid lscpu input: expected non-empty string, got {type(lscpu_data).__name__}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py` around lines 277 - 278, The function extract_numa_nodes currently returns [] on invalid input; make it fail fast like extract_primary_cpus by validating lscpu_data and raising the same exception type used in extract_primary_cpus (instead of returning an empty list). Locate extract_numa_nodes and replace the early return for non-string/empty lscpu_data with the same raise (e.g., ValueError or TypeError) and message pattern used in extract_primary_cpus to keep behavior and error messages consistent across extract_all_cpus, extract_primary_cpus, and extract_numa_nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@automation/test-execution/ansible/filter_plugins/cpu_utils.py`:
- Around line 302-304: The loop over range_list silently skips non-string values
(cpu_range) which masks templating bugs; update the iteration (the for idx,
cpu_range in enumerate(range_list) block) to continue only for empty strings but
raise a TypeError for any cpu_range that is not a str, including the offending
index and value in the error message so callers can diagnose the bad input
rather than producing an incomplete affinity mask.
- Around line 249-250: The function extract_all_cpus currently returns an empty
string on invalid input while extract_primary_cpus raises AnsibleFilterError;
update extract_all_cpus to mirror extract_primary_cpus by raising
AnsibleFilterError when lscpu_data is falsy or not a str instead of returning
"", i.e., replace the early-return in extract_all_cpus with a raise
AnsibleFilterError carrying a clear message about invalid lscpu input so both
functions fail fast and behave consistently.
- Around line 277-278: The function extract_numa_nodes currently returns [] on
invalid input; make it fail fast like extract_primary_cpus by validating
lscpu_data and raising the same exception type used in extract_primary_cpus
(instead of returning an empty list). Locate extract_numa_nodes and replace the
early return for non-string/empty lscpu_data with the same raise (e.g.,
ValueError or TypeError) and message pattern used in extract_primary_cpus to
keep behavior and error messages consistent across extract_all_cpus,
extract_primary_cpus, and extract_numa_nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e8ebc63-2229-4ca7-8890-9882ec80e1fd
📒 Files selected for processing (1)
automation/test-execution/ansible/filter_plugins/cpu_utils.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
LscpuParser class :
Error handling improvements
Best practices:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes