Conversation
📝 WalkthroughWalkthroughAdded GPU-directives cache initialization to the SlurmSystem pytest fixture and updated SlurmSystem tests: refactored one test to avoid a fixture, adjusted mocks, initialized cache state in a test, and added a new test for probe-error fallback behavior and logging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
Greptile SummaryThis PR improves test isolation by eliminating real Slurm system calls that were firing during the test suite — especially on systems with an accessible Slurm controller or inside sandboxed CI agents. The root cause was that Key changes:
Confidence Score: 5/5
Important Files Changed
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/systems/slurm/test_system.py (1)
19-19:⚠️ Potential issue | 🟡 MinorFix CI lint failure by removing unused typing imports.
Line 19 currently keeps
Dict/Listunused, and CI already reportsruff-checkauto-removing it. Please commit that cleanup so pre-commit passes.Suggested diff
-from typing import Dict, List🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/systems/slurm/test_system.py` at line 19, Remove the unused typing imports by deleting "Dict" and "List" from the typing import (the line "from typing import Dict, List"); if no other names are needed from typing, remove the entire import line, otherwise only keep the used symbols; save and run the linter (ruff/ruff-check) to confirm the unused-import warning is resolved and commit the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/systems/slurm/test_system.py`:
- Around line 163-169: The test currently calls
slurm_system.get_available_nodes_from_group(partition_name, group_name,
num_nodes) but discards its return; update the test to capture the return value
and assert it equals [] (empty list) to lock down fallback behavior — e.g., call
get_available_nodes_from_group(...) into a variable like result and assert
result == [] while keeping the existing mock and log assertions (refer to
get_available_nodes_from_group, partition_name, group_name, num_nodes).
---
Outside diff comments:
In `@tests/systems/slurm/test_system.py`:
- Line 19: Remove the unused typing imports by deleting "Dict" and "List" from
the typing import (the line "from typing import Dict, List"); if no other names
are needed from typing, remove the entire import line, otherwise only keep the
used symbols; save and run the linter (ruff/ruff-check) to confirm the
unused-import warning is resolved and commit the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45a3e744-8c4b-44bb-9e08-c015ab3b1879
📒 Files selected for processing (2)
tests/conftest.pytests/systems/slurm/test_system.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/systems/slurm/test_system.py (1)
162-171:⚠️ Potential issue | 🟡 MinorAssert fallback return value in the no-callstack test.
Line 162 calls
get_available_nodes_from_group(...)but discards the returned value. Capture it and assert[]so the fallback contract is explicitly locked.Suggested diff
- slurm_system.get_available_nodes_from_group(partition_name, group_name, num_nodes) + nodes = slurm_system.get_available_nodes_from_group(partition_name, group_name, num_nodes) + assert nodes == []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/systems/slurm/test_system.py` around lines 162 - 171, Capture the return value from the call to get_available_nodes_from_group(partition_name, group_name, num_nodes) and assert it equals [] to lock the fallback contract; e.g., assign the call to a variable (e.g., result) immediately after the invocation and add assert result == [] while keeping the existing mock assertions and log-message assertion intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/systems/slurm/test_system.py`:
- Around line 162-171: Capture the return value from the call to
get_available_nodes_from_group(partition_name, group_name, num_nodes) and assert
it equals [] to lock the fallback contract; e.g., assign the call to a variable
(e.g., result) immediately after the invocation and add assert result == []
while keeping the existing mock assertions and log-message assertion intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f54482bc-7047-4a12-b00d-ad4f2ea0fdf7
📒 Files selected for processing (1)
tests/systems/slurm/test_system.py
2 participants
Summary
Avoid real system calls. Most of them were related to
supports_gpu_directives_cache == None. It slows down tests on systems with slurm controller and in agents' sandbox.Test Plan
Additional Notes
–