Repository navigation
Conversation
Add register_cuda_log_bridge()/unregister_cuda_log_bridge() to cuda.core.utils, forwarding cuLogs* driver logs to a Python logging.Logger via cuLogsRegisterCallback/cuLogsUnregisterCallback. Refs: NVIDIA#671
register_cuda_log_bridge() now accepts an optional callback(logger, level, message) that overrides the default "[CUDA Driver] %s" forwarding. Non-callable callbacks raise TypeError. Add tests for callback override, level filtering, and validation. Refs: NVIDIA#671
…_error_log Matches CUDA's own Error Log Management terminology (cuLogs* API). Refs: NVIDIA#671
Also fix custom-callback test: CUlogLevel enum was referenced via a nonexistent driver.CULOG_LEVEL_* attribute instead of driver.CUlogLevel.CU_LOG_LEVEL_*. Refs NVIDIA#671
Also drop the stale userData TODO comment, move the CUlogLevel->logging level map into _default_callback (its only user), and rename its log_level param to cu_log_level for clarity. Refs NVIDIA#671
bytes already carries its own length via len(), so length added no information beyond what message already has. Updated the custom callback test to match the new 3-arg signature. Refs NVIDIA#671
Python closures already give callbacks access to enclosing state (e.g. logger), and cuLogsRegisterCallback is always registered with userData as None today, so it carried no information. Updated the custom callback test to match the new 2-arg signature. Refs NVIDIA#671
- Replace Chinese usage-comments with English equivalents. - Assert on exact message count/content instead of loose any() checks. - Add coverage for unregister() stopping further log forwarding. - Simplify the custom-callback test: single unconditional DEBUG rewrite instead of dead WARNING/ERROR branching. Refs NVIDIA#671
github-actions
Bot
added
the
cuda.core
📝 Summary
Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · up to The bridge tests can fail on supported driver and device configurations, and the public API is missing its required release-note entry. Address these before merging to keep the test workflow reliable and document the new API.
Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cuda_core/cuda/core/utils/__init__.py (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Add
utils.register_cuda_error_logandutils.unregister_cuda_error_logto thecuda.core.utilsAPI reference and to the 1.3.0 release notes. Document the default logger name"cuda.driver", replacement on re-registration, and that the underlyingcuLogs*driver API requires CUDA 12.9 or newer. Do not confuse this driver requirement with the separatecuda-bindings12.9.8 version floor.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
ce5d1f11-47d8-4648-ae02-f15452c3bab9
📒 Files selected for processing (3)
cuda_core/cuda/core/utils/__init__.pycuda_core/cuda/core/utils/_logging.pycuda_core/tests/test_cuda_log_bridge.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| @pytest.fixture(autouse=True) | ||
| def teardown_bridge(): | ||
| yield | ||
| # Unregister after every test so a leaked bridge can't leak into others. | ||
| unregister_cuda_error_log() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
important: Nothing in this module skips the tests when the driver is older than CUDA 12.9, where cuLogs* does not exist. On such drivers, register_cuda_error_log raises and every forwarding test fails instead of being skipped. The bindings test in cuda_bindings/tests/test_cuda.py:977-1007 already guards the same API with driver_version_less_than(12090). Add an equivalent module-level skipif here.
| # cuDeviceGet(9999) is out of range; driver writes two ERROR logs | ||
| with caplog.at_level(logging.ERROR, logger=logger_name): | ||
| driver.cuDeviceGet(9999) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- cuda_core/tests/AGENTS.md ---'
nl -ba cuda_core/tests/AGENTS.md
printf '%s\n' '--- test module ---'
nl -ba cuda_core/tests/test_cuda_log_bridge.py
printf '%s\n' '--- relevant conftest fixtures ---'
nl -ba cuda_core/tests/conftest.py | sed -n '210,245p'
printf '%s\n' '--- changed test diff ---'
git diff 0271e3ad33d56bd34530110a1decb8bebfdb0cb4 4e823c9fa263b0c817941be0e94887b71137abaf -- cuda_core/tests/test_cuda_log_bridge.pyRepository: NVIDIA/cuda-python
Length of output: 29840
important: Establish a current CUDA context for these driver tests.
driver.cuInit(0) initializes the driver but does not set a current context. The six tests that call driver.cuDeviceGet(9999) therefore violate cuda_core/tests/AGENTS.md and can inherit no or stale context. Add init_cuda independently of any skip for unsupported driver versions.
Suggested fix
-def test_cuda_driver_log_captured_via_caplog(caplog):
+def test_cuda_driver_log_captured_via_caplog(caplog, init_cuda):
-def test_cuda_driver_log_written_to_file(tmp_path):
+def test_cuda_driver_log_written_to_file(tmp_path, init_cuda):
-def test_cuda_driver_log_rebind_logger(caplog):
+def test_cuda_driver_log_rebind_logger(caplog, init_cuda):
-def test_cuda_driver_log_level_filtering(caplog):
+def test_cuda_driver_log_level_filtering(caplog, init_cuda):
-def test_cuda_driver_log_alert_counter():
+def test_cuda_driver_log_alert_counter(init_cuda):
-def test_cuda_driver_log_custom_callback_overrides_default(caplog):
+def test_cuda_driver_log_custom_callback_overrides_default(caplog, init_cuda):| assert len(messages) == 2, f"Expected exactly 2 driver log messages, got:\n{messages}" | ||
| assert "Parameter ordinal must be between 0 and 1" in messages[0] | ||
| assert "CUDA_ERROR_INVALID_DEVICE" in messages[1] and "cuDeviceGet" in messages[1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '24,85p' cuda_core/tests/test_cuda_log_bridge.pyRepository: NVIDIA/cuda-python
Length of output: 2482
🏁 Script executed:
rg -n -F -- 'unregister_cuda_error_log' cuda_core/tests/test_cuda_log_bridge.py
sed -n '1,210p' cuda_core/tests/test_cuda_log_bridge.pyRepository: NVIDIA/cuda-python
Length of output: 6606
important: Remove driver-specific log wording and count assumptions.
The driver does not guarantee diagnostic text or callback count. These assertions can fail without a bridge failure.
The post-unregister block already checks forwarding independently of message text with assert not any(r.name == logger_name for r in caplog.records). Keep that assertion and remove the redundant literal-text check.
Suggested fix
- # cuDeviceGet(9999) is out of range; driver writes two ERROR logs
+ # cuDeviceGet(9999) is out of range; the driver emits error logs
...
- assert len(messages) == 2, f"Expected exactly 2 driver log messages, got:\n{messages}"
- assert "Parameter ordinal must be between 0 and 1" in messages[0]
- assert "CUDA_ERROR_INVALID_DEVICE" in messages[1] and "cuDeviceGet" in messages[1]
+ assert messages, f"Expected forwarded driver log messages, got:\n{messages}"
...
- assert "Parameter ordinal must be between 0 and 1" not in caplog.textApply the same rule to the file, handler-count, and custom-callback checks: assert non-empty forwarded output or >= 1, not exact driver text or exactly two records.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert len(messages) == 2, f"Expected exactly 2 driver log messages, got:\n{messages}" | |
| assert "Parameter ordinal must be between 0 and 1" in messages[0] | |
| assert "CUDA_ERROR_INVALID_DEVICE" in messages[1] and "cuDeviceGet" in messages[1] | |
| assert messages, f"Expected forwarded driver log messages, got:\n{messages}" |
|
/ok to test 4e823c9 |
|
|
/ok to test 1f7e59a |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
0bd442cf-5439-4fb9-b769-bbe60e528a2a
📒 Files selected for processing (1)
cuda_core/docs/source/api.rst
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| :func:`~cuda.core.utils.register_cuda_error_log` bridges CUDA driver | ||
| ``cuLogs*`` messages into Python's standard :mod:`logging` module. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i 'register_cuda_error_log|unregister_cuda_error_log|driver logging|driver log' cuda_core/docs/source/releaseRepository: NVIDIA/cuda-python
Length of output: 156
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- release files ---'
rg --files cuda_core/docs/source/release
printf '%s\n' '--- logging-related release text ---'
rg -n -i 'log|cuda\.core\.utils|public api|api' cuda_core/docs/source/release || test "$?" -eq 1Repository: NVIDIA/cuda-python
Length of output: 7273
important: Add a release-note entry for register_cuda_error_log and unregister_cuda_error_log. The release notes contain no entry for this new public API or CUDA driver logging.
Source: Path instructions
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
Putting a hold on this to prevent accidental merge. The callback runs Python inside a driver callback and _cpp/rt/DESIGN.md ("Which channel to use") rules that out for driver callbacks to avoid deadlocks. The capture needs to be a GIL-free C function in the _cpp/rt layer.
More generally I think this feature is complex, so we need a functional spec with sign-off.
This branch has not been deployed
2 participants
Description
close #671
Adds
cuda.core.utils.register_cuda_error_log/unregister_cuda_error_log, a bridge that forwards CUDA drivercuLogs*messages (viacuLogsRegisterCallback) into Python's standardloggingmodule.register_cuda_error_log(logger=None, callback=None)registers a python callback with the driver that mapsCUlogLevelto the corresponding Python logging level and emits to alogging.Logger(defaults tologging.getLogger("cuda.driver")). An optionalcallbackcan override the default forwarding behavior entirely.unregister_cuda_error_log()tears down the registered callback.cuda.core.utils.Checklist