feat(c-api): add cuOptSetLogCallback and cuOptSetLogLevel - #1636
Open
ramakrishnap-nv wants to merge 3 commits into
Open
feat(c-api): add cuOptSetLogCallback and cuOptSetLogLevel#1636ramakrishnap-nv wants to merge 3 commits into
ramakrishnap-nv wants to merge 3 commits into
Conversation
Adds two new C API functions to address #1536 and #184: - cuOptSetLogCallback(settings, callback, user_data): registers a per-line log callback. Invoked once per log line in addition to any console/file sink already enabled. Pass NULL to clear. - cuOptSetLogLevel(settings, level): overrides log verbosity using the new CUOPT_LOG_LEVEL_* constants (TRACE…OFF) in constants.h. Internally, the callback is stored in solver_settings_handle_t and installed as a rapids_logger::callback_sink_mt via a RAII scope guard in cuOptSolve, so init_logger_t picks it up without any changes to the C++ internal solver-settings structs. The pending globals are protected by the existing g_guard_mutex that already serialises init_logger_t construction. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
📝 WalkthroughWalkthroughThe C API now supports per-solve log callbacks with user data, removes the solver log-level setter, and reads default log levels from ChangesSolver logging configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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: 2
🤖 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.
Inline comments:
In `@cpp/src/pdlp/cuopt_c.cpp`:
- Around line 1077-1096: Add GoogleTest cases covering cuOptSetLogCallback and
cuOptSetLogLevel: verify log callbacks receive messages, can be cleared, and
forward the configured user_data; reject levels outside CUOPT_LOG_LEVEL_TRACE
through CUOPT_LOG_LEVEL_OFF; and confirm the selected level is applied during
cuOptSolve. Reuse the existing C API test fixtures and solver setup.
In `@cpp/src/utilities/logger.cpp`:
- Around line 149-158: Make logging configuration solve-local: in
cpp/src/utilities/logger.cpp lines 149-158, store immutable callback, user-data,
and level state in the sink/logger guard and have user_log_bridge use that state
without reading mutable pending globals; in cpp/src/utilities/logger.cpp lines
209-222, apply configuration for every solve or explicitly serialize/reject
concurrent solve logging rather than reusing an existing guard; in
cpp/src/pdlp/cuopt_c.cpp lines 1174-1195, pass each solve’s logging
configuration directly into the logger lifecycle instead of publishing it
through shared globals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 493ccf78-693c-4957-a617-764507810f92
📒 Files selected for processing (5)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/src/pdlp/cuopt_c.cppcpp/src/utilities/logger.cppcpp/src/utilities/logger.hpp
Comment on lines
+1077
to
+1096
| cuopt_int_t cuOptSetLogCallback(cuOptSolverSettings settings, | ||
| cuOptLogCallback callback, | ||
| void* user_data) | ||
| { | ||
| if (settings == nullptr) { return CUOPT_INVALID_ARGUMENT; } | ||
| solver_settings_handle_t* handle = get_settings_handle(settings); | ||
| handle->log_callback = callback; | ||
| handle->log_callback_user_data = user_data; | ||
| return CUOPT_SUCCESS; | ||
| } | ||
|
|
||
| cuopt_int_t cuOptSetLogLevel(cuOptSolverSettings settings, int level) | ||
| { | ||
| if (settings == nullptr) { return CUOPT_INVALID_ARGUMENT; } | ||
| if (level < CUOPT_LOG_LEVEL_TRACE || level > CUOPT_LOG_LEVEL_OFF) { | ||
| return CUOPT_INVALID_ARGUMENT; | ||
| } | ||
| get_settings_handle(settings)->log_level = level; | ||
| return CUOPT_SUCCESS; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add coverage for the new C logging API.
Add GoogleTest coverage for callback delivery/clearing, user-data forwarding, invalid log levels, and level application during cuOptSolve. As per coding guidelines, contributors must add unit tests for code changes.
🤖 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 `@cpp/src/pdlp/cuopt_c.cpp` around lines 1077 - 1096, Add GoogleTest cases
covering cuOptSetLogCallback and cuOptSetLogLevel: verify log callbacks receive
messages, can be cleared, and forward the configured user_data; reject levels
outside CUOPT_LOG_LEVEL_TRACE through CUOPT_LOG_LEVEL_OFF; and confirm the
selected level is applied during cuOptSolve. Reuse the existing C API test
fixtures and solver setup.
Source: Coding guidelines
CI Test Summary✅ All 31 test job(s) passed. |
Fix a data race flagged in PR review: user_log_bridge was reading g_pending_callback (a mutable global) without any lock, creating a potential race against clear_pending_log_callback() called from cuOptSolve's RAII scope guard. Fix: capture the callback + user_data immutably into the logger_config_guard at init_logger_t construction time. The bridge now reads g_active_log_callback, a stable pointer into the guard's owned state. The pointer is only nulled after reset_default_logger() removes the sink (which blocks until all in-flight bridge calls complete), so there is no race window. Also adds a @warning to cuOptLogCallback documenting that log message formatting is not a stable API and the callback is intended for display purposes, not for parsing solver events programmatically. Tests added (c_api_test.c): - log_callback: verifies callback is invoked and user_data is forwarded - log_callback_cleared: verifies NULL callback produces no calls - log_level_off: verifies CUOPT_LOG_LEVEL_OFF silences all messages - log_level_invalid: verifies out-of-range levels are rejected Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@cpp/tests/linear_programming/c_api_tests/c_api_test.c`:
- Around line 377-390: Extend the callback lifecycle test around cuOptSolve to
first solve with the registered counting_log_callback and ctx, then create fresh
settings without a callback and solve again. Assert the second solve leaves
ctx.calls unchanged, while preserving the existing status checks and cleanup
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0ced68f-d15e-420b-ada2-0df32165076f
📒 Files selected for processing (5)
cpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/src/utilities/logger.cppcpp/tests/linear_programming/c_api_tests/c_api_test.ccpp/tests/linear_programming/c_api_tests/c_api_tests.cppcpp/tests/linear_programming/c_api_tests/c_api_tests.h
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/utilities/logger.cpp
- cpp/include/cuopt/mathematical_optimization/cuopt_c.h
chris-maes
reviewed
Jul 30, 2026
| /* @brief QCQP (barrier) scaling hyper-parameters */ | ||
| #define CUOPT_QCQP_HYPER_RUIZ_EQUILIBRATION "qcqp_hyper_ruiz_equilibration" | ||
|
|
||
| /* @brief Log level constants for cuOptSetLogLevel */ |
Contributor
There was a problem hiding this comment.
Are we sure we want to expose these log levels to the user?
Collaborator
Author
There was a problem hiding this comment.
We can always set this Error or warn by default and it is upto user what they want to check.
chris-maes
requested changes
Jul 30, 2026
chris-maes
left a comment
chris-maes
left a comment
Contributor
There was a problem hiding this comment.
I think we should discuss before merging. I'm supportive of adding an API like
cuOptSetLogCallback. But I don't think we should expose the different log levels to the user. These log levels are really more for developers and debugging. I'm fine with having an environmental variable that can change the log level.
Collaborator
Author
Updated, so removed set level and provided an option. in env to choose from existing levels. |
Per review (chris-maes): log levels are developer/debug-facing and should not be part of the public C API. Remove cuOptSetLogLevel and the public CUOPT_LOG_LEVEL_* constants; keep cuOptSetLogCallback. Verbosity is now controlled at runtime by the CUOPT_LOG_LEVEL environment variable (TRACE..OFF, case-insensitive), read in default_level(). - Point two mip_heuristics #if guards at RAPIDS_LOGGER_LOG_LEVEL_* instead of the removed CUOPT_LOG_LEVEL_*, fixing a pre-existing latent bug where those undefined macros evaluated to 0. - Replace the log-level unit tests with a cross-solve callback-lifecycle regression (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
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)
cpp/src/utilities/logger.cpp (1)
206-230: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBind callback configuration to each solve, not global pending state.
A concurrent solve can overwrite
g_pending_callbackafter another solve publishes it but before itsinit_logger_tconsumes it. Also, a secondinit_logger_treusesg_active_guard, so its logs can invoke the first solve’s callback/user data. This regresses the previously reported cross-solve configuration race.
cpp/src/utilities/logger.cpp#L206-L230: remove process-global pending callback ownership; capture configuration in a solve-scoped logger instance, or explicitly serialize logger configuration for the full solve lifetime.cpp/src/pdlp/cuopt_c.cpp#L1162-L1177: pass the callback configuration directly into that solve’s logger lifecycle rather than publishing it before initialization; add an overlapping-solves regression test.As per path instructions, “watch for thread-safety issues introduced by module-level/static pending callback storage (HIGH concurrency).”
🤖 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 `@cpp/src/utilities/logger.cpp` around lines 206 - 230, Remove the process-global pending callback state and bind callback configuration to each solve’s logger lifecycle. In cpp/src/utilities/logger.cpp lines 206-230, update set_pending_log_callback, clear_pending_log_callback, and related initialization so configuration is solve-scoped or logger configuration is serialized for the entire solve. In cpp/src/pdlp/cuopt_c.cpp lines 1162-1177, pass the callback and user data directly into that solve’s logger instead of publishing global state before initialization, and add an overlapping-solves regression test.Source: Path instructions
🧹 Nitpick comments (1)
cpp/src/utilities/logger.cpp (1)
90-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for
CUOPT_LOG_LEVEL.The replacement configuration path has no supplied test for case-insensitive valid values or unset/invalid fallback. Add isolated coverage for those behaviors.
🤖 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 `@cpp/src/utilities/logger.cpp` around lines 90 - 127, Add isolated tests for env_log_level covering case-insensitive recognized CUOPT_LOG_LEVEL values and confirming unset or unrecognized values return std::nullopt. Ensure each test controls and restores the environment variable so cases remain independent, and use the existing default_level behavior only where needed to verify fallback.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/src/utilities/logger.cpp`:
- Around line 206-230: Remove the process-global pending callback state and bind
callback configuration to each solve’s logger lifecycle. In
cpp/src/utilities/logger.cpp lines 206-230, update set_pending_log_callback,
clear_pending_log_callback, and related initialization so configuration is
solve-scoped or logger configuration is serialized for the entire solve. In
cpp/src/pdlp/cuopt_c.cpp lines 1162-1177, pass the callback and user data
directly into that solve’s logger instead of publishing global state before
initialization, and add an overlapping-solves regression test.
---
Nitpick comments:
In `@cpp/src/utilities/logger.cpp`:
- Around line 90-127: Add isolated tests for env_log_level covering
case-insensitive recognized CUOPT_LOG_LEVEL values and confirming unset or
unrecognized values return std::nullopt. Ensure each test controls and restores
the environment variable so cases remain independent, and use the existing
default_level behavior only where needed to verify fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0ce21bf-fa81-4a0b-9863-a2225b22dca2
📒 Files selected for processing (9)
cpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/feasibility_jump/feasibility_jump.cucpp/src/pdlp/cuopt_c.cppcpp/src/utilities/logger.cppcpp/src/utilities/logger.hppcpp/tests/linear_programming/c_api_tests/c_api_test.ccpp/tests/linear_programming/c_api_tests/c_api_tests.cppcpp/tests/linear_programming/c_api_tests/c_api_tests.h
💤 Files with no reviewable changes (1)
- cpp/src/utilities/logger.hpp
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
3 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.
Closes #1536, closes #184.
Adds a C API function plus an environment variable for solver log control:
cuOptSetLogCallback(settings, callback, user_data)— registers aper-line log callback invoked once per solver log line (in addition to
any console/file sink). Pass
NULLto clear.CUOPT_LOG_LEVELenvironment variable — overrides log verbosity atruntime (
TRACE,DEBUG,INFO,WARN,ERROR,CRITICAL,OFF,case-insensitive). Read in
default_level().Log level is intentionally not exposed as a public API (per review):
the level enum is developer/debug-facing, so verbosity is controlled via
the env var rather than a
cuOptSetLogLevelsetter. Note the env var isbounded by the compile-time
LIBCUOPT_LOGGING_LEVEL(defaultINFO) —lowering verbosity (e.g.
WARN/ERROR/OFF) always works; raising itabove the build level has no effect since those statements are compiled out.
Design: the callback is stored in
solver_settings_handle_tandinstalled as a
rapids_logger::callback_sink_mtvia a RAII scope guard incuOptSolve, soinit_logger_tpicks it up without changes to the C++internal solver-settings structs. It is captured immutably into the logger
guard to avoid a data race with the log bridge.