Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cpp/include/cuopt/mathematical_optimization/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@
/* @brief QCQP (barrier) scaling hyper-parameters */
#define CUOPT_QCQP_HYPER_RUIZ_EQUILIBRATION "qcqp_hyper_ruiz_equilibration"

/* @brief Log level constants for cuOptSetLogLevel */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want to expose these log levels to the user?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always set this Error or warn by default and it is upto user what they want to check.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed — no log level is exposed through the API at all now, per your Slack comments as well.

  • The constants.h level enum and cuOptSetLogLevel are gone; this PR no longer touches constants.h.
  • The callback itself no longer carries severity:
-typedef void (*cuOptLogCallback)(int level, const char* message, void* user_data);
+typedef void (*cuOptLogCallback)(const char* message, void* user_data);

There is now no enum, no numeric mapping, and nothing for callers to branch on. The previous version tried to handle this with a doc comment ("not a stable programmatic API"), which does not stop code being written against a value it can see.

  • The bridge also filters below info, so debug and trace never reach user code. That previously held only by accident of the build — CUOPT_LOG_ACTIVE_LEVEL defaults to INFO so those statements are compiled out, but a lower-level build, or CUOPT_LOG_LEVEL=DEBUG against one, would have routed internal diagnostics into a user callback.

One judgement call worth your view. You said the callback should fire only on a standard log line and that no code should see our debug and info messages. Those cannot both hold literally here: cuOpt's standard solver output is emitted at infoCUOPT_LOG_INFO covers "Solve status", "PDLP finished" and the progress table, roughly 153 call sites (level census: TRACE 109, DEBUG 295, INFO 153, WARN 13, ERROR 46). Filtering info out would leave the callback with essentially nothing to deliver.

So I read "debug and info" as internal diagnostics and cut at info: callers get what the console would show, and nothing below it. If you meant something stricter — a dedicated user-log channel separate from info diagnostics — that means auditing those ~153 sites and splitting them, which I would rather do as a follow-up than fold in here. Happy to go that way if you prefer.

#define CUOPT_LOG_LEVEL_TRACE 0
#define CUOPT_LOG_LEVEL_DEBUG 1
#define CUOPT_LOG_LEVEL_INFO 2
#define CUOPT_LOG_LEVEL_WARN 3
#define CUOPT_LOG_LEVEL_ERROR 4
#define CUOPT_LOG_LEVEL_CRITICAL 5
#define CUOPT_LOG_LEVEL_OFF 6

/* @brief MIP determinism mode constants */
#define CUOPT_MODE_OPPORTUNISTIC 0
#define CUOPT_MODE_DETERMINISTIC 1
Expand Down
39 changes: 39 additions & 0 deletions cpp/include/cuopt/mathematical_optimization/cuopt_c.h
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,45 @@ cuopt_int_t cuOptGetFloatParameter(cuOptSolverSettings settings,
const char* parameter_name,
cuopt_float_t* parameter_value);

/**
* @brief Type of callback invoked once per log line emitted by the solver.
*
* @param level Log level (one of CUOPT_LOG_LEVEL_*).
* @param message Null-terminated log line without trailing newline.
* @param user_data Opaque pointer passed to cuOptSetLogCallback.
*
* @note The callback is invoked from the solver thread. Do not call back into
* cuOpt from inside the callback.
*/
typedef void (*cuOptLogCallback)(int level, const char* message, void* user_data);

/**
* @brief Register a callback to receive solver log messages.
*
* The callback is invoked once per log line. It is called in addition to any
* file or console sink already enabled via ``log_to_console`` / ``log_file``
* parameters. Pass NULL to remove a previously registered callback.
*
* @param[in] settings The solver settings object.
* @param[in] callback Callback function, or NULL to clear.
* @param[in] user_data Opaque pointer forwarded to the callback unchanged.
*
* @return A status code indicating success or failure.
*/
cuopt_int_t cuOptSetLogCallback(cuOptSolverSettings settings,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this handled with GRPC?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — it wasn't, and it is now (30632ca).

With CUOPT_REMOTE_HOST/CUOPT_REMOTE_PORT set, cuOptSolve dispatches through solve_lp_remote / solve_mip_remote, and the callback saw almost nothing. Two reasons:

  1. The streamed server log was written straight to std::cout and the log file, bypassing default_logger() and therefore the callback sink.
  2. config.stream_logs was only enabled when log_to_console or log_file was set, so a registered callback alone did not even request streaming.

Both remote paths now forward each streamed line to the user callback, and request streaming when a callback is present. The registration is captured at call time rather than read inside the lambda, because grpc_client_t invokes log_callback from its own log_thread_, which carries no thread-local registration (registration is per-thread as of 6562dda, so concurrent solves cannot capture each other's callback).

Verified against a live cuopt_grpc_server with a small C probe — the same LP delivers the same number of callback lines either way:

local :  solve rc=0 callback_calls=16
remote:  solve rc=0 callback_calls=16

and the remote lines are the server's own solver log (Using PSLP presolver, PSLP Presolved problem: ..., Status: Optimal), cross-checked against the server-side log.

One behavioural note now documented in the header: the callback runs on the calling thread for a local solve, and on the log-streaming thread for a remote one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test for this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: this is now covered by a test as well (31e6c10).

CpuOnlyWithServerTest.log_callback_remote reuses the existing fixture that forks a real cuopt_grpc_server and points CUOPT_REMOTE_HOST/PORT at it.

Worth noting why the assertion is what it is: the client emits its own Using remote GPU backend line locally, so a bare "callback fired at least once" check passes even when no server output is forwarded at all. The test therefore asserts that a line from the server's solver log arrives. Reverting the forwarding in solve_remote.cpp makes it fail with:

Remote solve delivered 1 lines but none from the server's solver log
[  FAILED  ] CpuOnlyWithServerTest.log_callback_remote

cuOptLogCallback callback,
void* user_data);

/**
* @brief Set the solver log verbosity level.
*
* @param[in] settings The solver settings object.
* @param[in] level One of CUOPT_LOG_LEVEL_TRACE … CUOPT_LOG_LEVEL_OFF.
*
* @return A status code indicating success or failure.
*/
cuopt_int_t cuOptSetLogLevel(cuOptSolverSettings settings, int level);

/**
* @brief Type of callback for receiving incumbent MIP solutions with user context.
*
Expand Down
49 changes: 49 additions & 0 deletions cpp/src/pdlp/cuopt_c.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ struct solver_settings_handle_t {
~solver_settings_handle_t() { delete settings; }
solver_settings_t<cuopt_int_t, cuopt_float_t>* settings;
std::vector<std::unique_ptr<cuopt::internals::base_solution_callback_t>> callbacks;
// Log callback registered via cuOptSetLogCallback
cuOptLogCallback log_callback{nullptr};
void* log_callback_user_data{nullptr};
// Log level override registered via cuOptSetLogLevel (-1 = use default)
int log_level{-1};
};

solver_settings_handle_t* get_settings_handle(cuOptSolverSettings settings)
Expand Down Expand Up @@ -1069,6 +1074,27 @@ cuopt_int_t cuOptSetMIPSetSolutionCallback(cuOptSolverSettings settings,
return CUOPT_SUCCESS;
}

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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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


cuopt_int_t cuOptSetInitialPrimalSolution(cuOptSolverSettings settings,
const cuopt_float_t* primal_solution,
cuopt_int_t num_variables)
Expand Down Expand Up @@ -1145,6 +1171,29 @@ cuopt_int_t cuOptSolve(cuOptOptimizationProblem problem,
if (settings == nullptr) { return CUOPT_INVALID_ARGUMENT; }
if (solution_ptr == nullptr) { return CUOPT_INVALID_ARGUMENT; }

// Install user log callback / level so init_logger_t inside the solver picks them up.
// The RAII guard clears them on scope exit (whether by return or exception).
solver_settings_handle_t* handle = get_settings_handle(settings);
struct log_scope_guard_t {
bool has_callback;
bool has_level;
~log_scope_guard_t()
{
if (has_callback) { cuopt::clear_pending_log_callback(); }
if (has_level) { cuopt::clear_pending_log_level(); }
}
} log_scope{false, false};

if (handle->log_callback) {
// cuOptLogCallback and log_callback_with_data_t share the same signature.
cuopt::set_pending_log_callback(handle->log_callback, handle->log_callback_user_data);
log_scope.has_callback = true;
}
if (handle->log_level >= 0) {
cuopt::set_pending_log_level(handle->log_level);
log_scope.has_level = true;
}

problem_and_stream_view_t* problem_and_stream_view =
static_cast<problem_and_stream_view_t*>(problem);

Expand Down
45 changes: 45 additions & 0 deletions cpp/src/utilities/logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,43 @@ struct logger_config_guard {
static std::weak_ptr<logger_config_guard> g_active_guard;
static std::mutex g_guard_mutex;

// Pending user log callback set by the C API before a solve.
// Accessed under g_guard_mutex.
static log_callback_with_data_t g_pending_callback = nullptr;
static void* g_pending_callback_data = nullptr;
static int g_pending_log_level = -1; // -1 = use compiled default

static void user_log_bridge(int lvl, const char* msg)
{
if (g_pending_callback) { g_pending_callback(lvl, msg, g_pending_callback_data); }
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

void set_pending_log_callback(log_callback_with_data_t cb, void* user_data)
{
std::lock_guard<std::mutex> lock(g_guard_mutex);
g_pending_callback = cb;
g_pending_callback_data = user_data;
}

void clear_pending_log_callback()
{
std::lock_guard<std::mutex> lock(g_guard_mutex);
g_pending_callback = nullptr;
g_pending_callback_data = nullptr;
}

void set_pending_log_level(int level)
{
std::lock_guard<std::mutex> lock(g_guard_mutex);
g_pending_log_level = level;
}

void clear_pending_log_level()
{
std::lock_guard<std::mutex> lock(g_guard_mutex);
g_pending_log_level = -1;
}

init_logger_t::init_logger_t(std::string log_file, bool log_to_console)
{
std::lock_guard<std::mutex> lock(g_guard_mutex);
Expand All @@ -169,13 +206,21 @@ init_logger_t::init_logger_t(std::string log_file, bool log_to_console)
std::make_shared<rapids_logger::basic_file_sink_mt>(log_file, true));
cuopt::default_logger().flush_on(rapids_logger::level_enum::debug);
}
if (g_pending_callback) {
cuopt::default_logger().sinks().push_back(
std::make_shared<rapids_logger::callback_sink_mt>(user_log_bridge));
}

#if CUOPT_LOG_ACTIVE_LEVEL >= RAPIDS_LOGGER_LOG_LEVEL_INFO
cuopt::default_logger().set_pattern("%v");
#else
cuopt::default_logger().set_pattern(cuopt::default_pattern());
#endif

if (g_pending_log_level >= 0) {
cuopt::default_logger().set_level(static_cast<rapids_logger::level_enum>(g_pending_log_level));
}

// Extract messages from the global buffer and log to the default logger
auto buffered_messages = global_log_buffer().drain_all();
for (const auto& entry : buffered_messages) {
Expand Down
20 changes: 20 additions & 0 deletions cpp/src/utilities/logger.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,26 @@ rapids_logger::logger& default_logger();
*/
void reset_default_logger();

// C-compatible log callback type: void callback(int level, const char* msg, void* user_data)
// Matches cuOptLogCallback in cuopt_c.h — layout-compatible, no dependency on that header.
using log_callback_with_data_t = void (*)(int level, const char* message, void* user_data);

/**
* @brief Install a user log callback to be picked up by the next init_logger_t.
*
* Must be called before the init_logger_t that starts the targeted solve.
* Protected by the same mutex as init_logger_t so it is safe to call from
* any thread, but do not call from inside the callback itself.
*/
void set_pending_log_callback(log_callback_with_data_t cb, void* user_data);
void clear_pending_log_callback();

/**
* @brief Override the log level for the next init_logger_t. Pass -1 to restore the default.
*/
void set_pending_log_level(int level);
void clear_pending_log_level();

// Ref-counted logger initializer
class init_logger_t {
// Using shared_ptr for ref-counting
Expand Down
Loading