[cuda] fix: align visible mask prefix parsing - #221
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR changes CUDA_VISIBLE_DEVICES mask parsing to accept unique UUID prefixes and stop parsing at ChangesCUDA UUID-prefix and -1 truncation parity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NVMLMonitor
participant lookup_nvml_uuid_handle
participant NVML
participant lookup_nvml_uuid_prefix_handle
NVMLMonitor->>lookup_nvml_uuid_handle: resolve token from CUDA_VISIBLE_DEVICES
lookup_nvml_uuid_handle->>NVML: nvmlDeviceGetHandleByUUID(token)
alt direct match found
NVML-->>lookup_nvml_uuid_handle: handle
else lookup unsupported/failed
lookup_nvml_uuid_handle->>lookup_nvml_uuid_prefix_handle: fallback with token
lookup_nvml_uuid_prefix_handle->>NVML: enumerate device UUIDs
NVML-->>lookup_nvml_uuid_prefix_handle: UUID list
alt exactly one prefix match
lookup_nvml_uuid_prefix_handle-->>lookup_nvml_uuid_handle: handle
else zero or multiple matches
lookup_nvml_uuid_prefix_handle-->>lookup_nvml_uuid_handle: None
end
end
lookup_nvml_uuid_handle-->>NVMLMonitor: handle or None (unavailable telemetry)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
🧹 Nitpick comments (2)
src/keep_gpu/utilities/gpu_info.py (1)
115-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
try/exceptaroundlookup_nvml_uuid_handlecall.
lookup_nvml_uuid_handle(pynvml, token)is called withoutshould_reraise, so internally it never re-raises (all internal exceptions are swallowed and it returnsNone). The wrappingtry/except Exception: handle = Noneat Line 122-125 is therefore effectively dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/keep_gpu/utilities/gpu_info.py` around lines 115 - 157, The try/except around lookup_nvml_uuid_handle in _resolve_nvml_visible_handles is redundant because that helper already swallows internal failures and returns None when should_reraise is not set. Remove the dead exception handling and keep the existing None check/early return flow, leaving the rest of the handle resolution logic unchanged.tests/utilities/test_gpu_monitor.py (1)
424-477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
list(range(8))magic literal across assertions.
dummy.queried_indexes == list(range(8))appears at lines 318, 430, and 488, hardcoding theDummyNVMLdefaultcount=8. Deriving it fromdummy.countwould keep the assertions correct if the default ever changes.♻️ Example
- assert dummy.queried_indexes == list(range(8)) + assert dummy.queried_indexes == list(range(dummy.count))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utilities/test_gpu_monitor.py` around lines 424 - 477, The repeated hardcoded queried-index assertion in the NVML monitor tests should be derived from the dummy’s configured count instead of assuming eight. Update the assertions in the affected test cases under test_gpu_monitor.py to use the DummyNVML instance’s count (or an equivalent shared value) when checking NVMLMonitor.get_gpu_utilization behavior, so the expectation stays aligned with the DummyNVML default and any future count changes. Use the existing DummyNVML and NVMLMonitor symbols to locate and adjust the repeated assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/keep_gpu/utilities/gpu_info.py`:
- Around line 115-157: The try/except around lookup_nvml_uuid_handle in
_resolve_nvml_visible_handles is redundant because that helper already swallows
internal failures and returns None when should_reraise is not set. Remove the
dead exception handling and keep the existing None check/early return flow,
leaving the rest of the handle resolution logic unchanged.
In `@tests/utilities/test_gpu_monitor.py`:
- Around line 424-477: The repeated hardcoded queried-index assertion in the
NVML monitor tests should be derived from the dummy’s configured count instead
of assuming eight. Update the assertions in the affected test cases under
test_gpu_monitor.py to use the DummyNVML instance’s count (or an equivalent
shared value) when checking NVMLMonitor.get_gpu_utilization behavior, so the
expectation stays aligned with the DummyNVML default and any future count
changes. Use the existing DummyNVML and NVMLMonitor symbols to locate and adjust
the repeated assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a2db652-3829-47f8-86c5-166532deba00
📒 Files selected for processing (13)
AGENTS.mddocs/concepts/architecture.mddocs/getting-started.mddocs/guides/mcp.mddocs/guides/python.mddocs/plans/cuda-visible-prefix-mask-parity.mddocs/reference/api.mddocs/reference/cli.mdsrc/keep_gpu/utilities/cuda_visibility.pysrc/keep_gpu/utilities/gpu_info.pysrc/keep_gpu/utilities/gpu_monitor.pytests/utilities/test_gpu_info.pytests/utilities/test_gpu_monitor.py
9436a1b to
4e3b222
Compare
1 participant
Summary
CUDA_VISIBLE_DEVICESat-1after any valid prefix.cuda_visibility.pyand share it across GPU listing and utilization telemetry.Local Review
Test Plan
-1truncation tests failed as expected.PYTHONPATH=$PWD/src pytest tests/utilities/test_gpu_info.py tests/utilities/test_gpu_monitor.py -q->83 passed, 1 skippedPYTHONPATH=$PWD/src pytest -q->842 passed, 11 skippedpre-commit run --all-files --show-diff-on-failure-> passedPYTHONPATH=$PWD/src mkdocs build --strict-> passed with the known Material for MkDocs 2.0 warninggit diff --check-> passedSummary by CodeRabbit
New Features
-1after valid entries.Bug Fixes