Fix bug in logger when log_to_console is set to false #538
Conversation
rg20
requested review from
akifcorduk,
aliceb-nv and
tmckayus
and removed request for
a team
rg20
added
bug
WalkthroughSwapped many includes from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cpp/src/utilities/logger.cpp (2)
23-52: Consider making the messages vector private.The
messagesmember at line 50 is public, which breaks encapsulation. While the accessor methods properly use the mutex, direct access tomessageswould bypass thread safety.Apply this diff:
- std::vector<std::string> messages; - mutable std::mutex mutex; + private: + std::vector<std::string> messages; + mutable std::mutex mutex;Note: The buffer could grow unbounded if logger initialization is significantly delayed. Consider adding a maximum buffer size or warning if this becomes an issue in practice.
142-175: Consider protecting against concurrent initialization.The
init_logger_tconstructor and destructor modify the globaldefault_logger()sinks without synchronization. If multiple instances are created concurrently (e.g., in a multi-threaded test environment), race conditions could occur:
- Line 146:
pop_back()could be called twice, causing undefined behavior if the sinks vector becomes empty- Lines 150-157: Multiple threads could add duplicate sinks
- Line 175: Destructor resets logger, potentially interfering with other active instances
While typical usage (one solver per process) may not trigger this, consider adding:
- A static mutex to protect initialization
- A reference count to prevent premature reset in the destructor
- Documentation noting that
init_logger_tshould not be used concurrentlyBased on learnings: The PR objectives mention this is for initialization at solver setup, so concurrent usage may be unlikely in practice.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
benchmarks/linear_programming/cuopt/run_mip.cpp(1 hunks)cpp/cuopt_cli.cpp(1 hunks)cpp/src/CMakeLists.txt(1 hunks)cpp/src/dual_simplex/logger.hpp(1 hunks)cpp/src/linear_programming/cuopt_c.cpp(1 hunks)cpp/src/linear_programming/optimization_problem.cu(1 hunks)cpp/src/linear_programming/solve.cu(1 hunks)cpp/src/linear_programming/solver_settings.cu(1 hunks)cpp/src/linear_programming/solver_solution.cu(1 hunks)cpp/src/linear_programming/utilities/logger_init.hpp(0 hunks)cpp/src/math_optimization/solution_writer.cu(1 hunks)cpp/src/math_optimization/solver_settings.cu(1 hunks)cpp/src/mip/logger.cuh(1 hunks)cpp/src/mip/logger.hpp(1 hunks)cpp/src/mip/presolve/third_party_presolve.cpp(1 hunks)cpp/src/mip/solve.cu(1 hunks)cpp/src/mip/solver_solution.cu(1 hunks)cpp/src/routing/solve.cu(1 hunks)cpp/src/utilities/logger.cpp(5 hunks)cpp/src/utilities/logger.hpp(1 hunks)cpp/src/utilities/version_info.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- cpp/src/linear_programming/utilities/logger_init.hpp
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/src/utilities/logger.hpp (1)
cpp/src/dual_simplex/logger.hpp (2)
log_file(43-48)log_file(56-59)
🪛 Clang (14.0.6)
cpp/src/utilities/logger.hpp
[error] 20-20: 'cuopt/logger_macros.hpp' file not found
(clang-diagnostic-error)
cpp/src/mip/logger.hpp
[error] 20-20: 'utilities/logger.hpp' file not found
(clang-diagnostic-error)
cpp/src/utilities/logger.cpp
[error] 18-18: 'utilities/logger.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (20)
cpp/cuopt_cli.cpp (1)
22-22: LGTM! Header migration aligns with logging consolidation.The include path update from
cuopt/logger.hpptoutilities/logger.hppis consistent with the project-wide refactoring to centralize logging infrastructure and fix the early initialization issue described in the PR objectives.cpp/src/linear_programming/solver_solution.cu (1)
21-21: LGTM! Logging header migration is correct.The header path update aligns with the centralized logging infrastructure introduced in this PR.
cpp/src/math_optimization/solver_settings.cu (1)
20-20: LGTM! Header migration enables proper parameter logging.This header update is particularly important for the PR objective, as this file logs parameter changes (lines 148, 166, 181, 195, etc.). Migrating to
utilities/logger.hppensures these parameter logs are buffered until the logger is properly initialized, preventing premature stdout output whenLOG_TO_CONSOLEis false.cpp/src/linear_programming/optimization_problem.cu (1)
20-20: LGTM! Header path update is correct.The migration to
utilities/logger.hppis consistent with the project-wide logging consolidation effort.cpp/src/utilities/version_info.cpp (1)
22-22: LGTM! Header migration fixes early version logging.The header update is correct. Since
print_version_info()is called early during solver initialization, this change ensures version information logs are properly buffered and respect theLOG_TO_CONSOLEsetting.benchmarks/linear_programming/cuopt/run_mip.cpp (1)
26-26: LGTM! Benchmark code updated consistently.The header migration is correct and aligns with the logging infrastructure changes across the project.
cpp/src/linear_programming/solve.cu (1)
25-25: LGTM! Logger initialization header consolidation is correct.The change from
linear_programming/utilities/logger_init.hpptoutilities/logger.hppconsolidates the logger initialization API. Line 799 showsinit_logger_tbeing instantiated with settings, which is the key to ensuring logs are properly initialized before parameter logging occurs.cpp/src/math_optimization/solution_writer.cu (1)
19-19: LGTM! Header migration complete.The include path update is correct and completes the consistent migration to
utilities/logger.hppacross the codebase.cpp/src/routing/solve.cu (1)
20-20: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation.
cpp/src/mip/logger.cuh (1)
20-20: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation.
cpp/src/CMakeLists.txt (1)
17-17: LGTM! Build configuration updated for new logger implementation.The change from
logger_helper.cpptologger.cppreflects the logger infrastructure refactoring that introduces the log buffering mechanism.cpp/src/mip/presolve/third_party_presolve.cpp (1)
22-22: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation.
cpp/src/mip/logger.hpp (1)
20-20: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation. The static analysis warning about the missing file is likely a false positive since the CMakeLists.txt updates reference the new logger implementation files.
Please verify that the codebase compiles successfully with these header changes.
cpp/src/mip/solver_solution.cu (1)
20-20: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation.
cpp/src/linear_programming/solver_settings.cu (1)
24-24: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation.
cpp/src/dual_simplex/logger.hpp (1)
21-21: LGTM! Consistent header migration.The include path update aligns with the repository-wide logging infrastructure consolidation. Note that this file conditionally includes the logger only when
CUOPT_LOG_ACTIVE_LEVELis defined, which is appropriate for build-time log level control.cpp/src/mip/solve.cu (1)
32-32: LGTM: Include path updated to new logger infrastructure.The change from
linear_programming/utilities/logger_init.hpptoutilities/logger.hppaligns with the centralized logging refactor.cpp/src/linear_programming/cuopt_c.cpp (1)
24-24: LGTM: Include path updated to new logger module.The change aligns with the project-wide logger refactor to use
utilities/logger.hpp.cpp/src/utilities/logger.hpp (1)
18-51: LGTM: Clean public API for logger management.The header provides a minimal, well-designed public interface with RAII-based logger initialization. The
init_logger_tclass will ensure proper cleanup when it goes out of scope.cpp/src/utilities/logger.cpp (1)
142-173: Verify behavior when bothlog_to_consoleis false andlog_fileis empty.The constructor unconditionally removes the default buffer sink (line 146), then conditionally adds console and file sinks. If both
log_to_consoleis false andlog_fileis empty, no sink will be added, causing subsequent log messages to be discarded silently.Please verify this is the intended behavior. If not, consider either:
- Keeping at least one sink active, or
- Adding a comment documenting that this configuration silently discards logs
Also address the commented-out code (lines 144-147) and TODO (line 154) that suggest this logic is still under consideration, and remove the commented debug output at line 168.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cpp/src/utilities/logger.cpp (1)
163-169: Current flush replays everything as info and can drop messages.Given the earlier refactor, adopt drain_prefix + level-preserving replay to meet correctness and avoid loss.
🧹 Nitpick comments (4)
cpp/src/utilities/logger.cpp (4)
67-78: Update stale docstring and change-of-behavior notice.Comment says default sink uses CUOPT_DEBUG_LOG_FILE or stderr; code now always uses a callback sink. Update docs to reflect buffering approach. Also note console sink uses stdout (ostream_sink_mt to std::cout) vs historical stderr.
- * If the environment variable `CUOPT_DEBUG_LOG_FILE` is defined, the default sink is a sink to that - * file. Otherwise, the default is to dump to stderr. + * Prior to explicit initialization, the default sink is a callback sink that buffers messages + * in-memory (no console/file output). After init, messages are routed to configured sinks.
129-140: Reset logic is OK, but consider removing callback sink by type, not position.If sinks were reordered elsewhere, push_back(default_sink()) could duplicate callback sinks. Not urgent but safer to rebuild sink list deterministically.
171-172: Destructor resets to buffered mode; confirm lifecycle.Resetting the global logger in ~init_logger_t() may surprise downstream code if the RAII object goes out of scope early. Consider making reset explicit or documenting that init_logger_t must live for process lifetime.
Do you expect init_logger_t to be created once during startup and live until shutdown?
147-155: Console to stdout vs stderr.Historically comments mention stderr; code uses std::cout. Confirm product expectation. If parity with prior behavior is desired, use std::cerr.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/utilities/logger.cpp(5 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/utilities/logger.cpp
[error] 18-18: 'utilities/logger.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
cpp/src/utilities/logger.cpp (2)
111-127: Default logger setup looks fine.Pattern/level wiring via compile-time macros is consistent; returns a stable singleton.
18-18: ****The header
utilities/logger.hppexists atcpp/src/utilities/logger.hppand is properly configured in the build system. The source filecpp/src/utilities/logger.cppis compiled as part of the cuopt target (added viacpp/src/CMakeLists.txtline 17), which has${CMAKE_CURRENT_SOURCE_DIR}/srcin its PRIVATE include directories. When a source file is compiled as part of a target, it has access to that target's PRIVATE includes, so the angle-bracket include#include <utilities/logger.hpp>resolves correctly. No build error exists.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/utilities/logger.cpp (1)
150-179: Critical initialization race conditions remain unresolved from previous review.The constructor still has the race conditions identified in the previous review:
- Zero-sink window (line 154):
clear()removes all sinks, including the buffer sink. Between the clear and adding new sinks (lines 158-166), any logs are dropped.- No serialization: Multiple concurrent
init_logger_tconstructions can race. The class is not a singleton (perlogger.hpplines 46–51), so concurrent instantiation is possible.- Buffer sink removal: When sinks are cleared at line 154, the buffer callback sink is also removed. Any logs arriving between clear and drain (line 175) are lost because there's no sink to receive them.
The previous review provided a detailed solution involving:
- A static mutex to serialize logger reconfiguration
- Adding new sinks before removing the buffer sink
- Removing only the callback sink (not clearing all sinks)
This approach ensures no log-loss window exists.
Apply a minimal fix to prevent the zero-sink window:
init_logger_t::init_logger_t(std::string log_file, bool log_to_console) { - // until this function is called, the default sink is the buffer sink - cuopt::default_logger().sinks().clear(); - - // re-initialize sinks + auto& logger = cuopt::default_logger(); + + // Add new sinks first to avoid zero-sink window if (log_to_console) { - cuopt::default_logger().sinks().push_back( - std::make_shared<rapids_logger::ostream_sink_mt>(std::cout)); + logger.sinks().push_back(std::make_shared<rapids_logger::ostream_sink_mt>(std::cout)); } if (!log_file.empty()) { - cuopt::default_logger().sinks().push_back( - std::make_shared<rapids_logger::basic_file_sink_mt>(log_file, true)); - cuopt::default_logger().flush_on(rapids_logger::level_enum::debug); + logger.sinks().push_back(std::make_shared<rapids_logger::basic_file_sink_mt>(log_file, true)); + logger.flush_on(rapids_logger::level_enum::debug); } #if CUOPT_LOG_ACTIVE_LEVEL >= RAPIDS_LOGGER_LOG_LEVEL_INFO - cuopt::default_logger().set_pattern("%v"); + logger.set_pattern("%v"); #else - cuopt::default_logger().set_pattern(cuopt::default_pattern()); + logger.set_pattern(cuopt::default_pattern()); #endif // 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) { - cuopt::default_logger().log(entry.level, entry.msg.c_str()); + logger.log(entry.level, entry.msg.c_str()); } + + // Remove buffer sink only after draining and with new sinks in place + auto& sinks = logger.sinks(); + sinks.erase(std::remove_if(sinks.begin(), sinks.end(), + [](const rapids_logger::sink_ptr& sp) { + return std::dynamic_pointer_cast<rapids_logger::callback_sink_mt>(sp) != nullptr; + }), + sinks.end()); }Note: For full thread-safety with concurrent
init_logger_tconstructions, add a static mutex as detailed in the previous review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/utilities/logger.cpp(5 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/utilities/logger.cpp
[error] 18-18: 'utilities/logger.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (2)
cpp/src/utilities/logger.cpp (2)
23-60: Excellent implementation of buffered logging with level preservation.The
log_bufferclass now correctly addresses the concerns from previous reviews:
buffered_entrystruct preserves both level and messagelog()method null-guardsmsgand stores the severitydrain_all()provides atomic swap semantics, eliminating the race between get/clear- Thread-safe operations with proper mutex usage
68-73: Callback correctly preserves log severity.The callback now properly casts the level and forwards it to the buffer, addressing the previous review concern about dropped severity information.
|
/ok to test b0c872e |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cpp/src/utilities/logger.cpp (1)
153-164: Zero-sink window can drop logs; consider adding sinks before clearing.Between
clear()(line 153) and adding the console/file sinks (lines 156–164), the logger has no sinks. Any log emitted by another thread during this brief window will be silently dropped.While this may be acceptable if
init_logger_tis only constructed once during single-threaded startup, a safer approach is to add the new sinks before clearing the old ones.Example safer order:
- cuopt::default_logger().sinks().clear(); - - // re-initialize sinks + // re-initialize sinks + auto& logger = cuopt::default_logger(); + + // Add new sinks first to avoid a zero-sink window if (log_to_console) { - cuopt::default_logger().sinks().push_back( + logger.sinks().push_back( std::make_shared<rapids_logger::ostream_sink_mt>(std::cout)); } if (!log_file.empty()) { - cuopt::default_logger().sinks().push_back( + logger.sinks().push_back( std::make_shared<rapids_logger::basic_file_sink_mt>(log_file, true)); - cuopt::default_logger().flush_on(rapids_logger::level_enum::debug); + logger.flush_on(rapids_logger::level_enum::debug); } + + // Now remove the buffer sink after new sinks are in place + auto& sinks = logger.sinks(); + sinks.erase( + std::remove_if(sinks.begin(), sinks.end(), + [](const auto& sp) { + return std::dynamic_pointer_cast<rapids_logger::callback_sink_mt>(sp) != nullptr; + }), + sinks.end());
🧹 Nitpick comments (1)
cpp/src/utilities/logger.cpp (1)
58-59: Consider making data members private.Public
messagesandmutexbreak encapsulation. While the current API doesn't expose direct mutation, making them private would prevent future misuse.Apply this diff:
+ private: std::vector<buffered_entry> messages; mutable std::mutex mutex; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/utilities/logger.cpp(5 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/utilities/logger.cpp
[error] 18-18: 'utilities/logger.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (6)
cpp/src/utilities/logger.cpp (6)
23-26: LGTM—severity preservation addressed.The
buffered_entrystruct correctly stores both level and message, addressing the previous concern about losing severity information during buffering.
34-56: Excellent—atomic drain eliminates the get+clear race.The implementation correctly addresses previous concerns:
log()preserves severity and null-guards the message.drain_all()atomically extracts messages viaswap(), preventing the race between separate get/clear calls.
62-73: LGTM—callback preserves severity.The singleton pattern is thread-safe, and
buffer_log_callbackcorrectly casts and forwards the level to the buffer, addressing the previous "FIXME: check for levels?" comment.
172-176: LGTM—buffered messages replayed with original severity.The drain-and-replay logic correctly uses
entry.levelto preserve each message's severity, addressing the prior concern about replaying everything as info.
179-179: LGTM—cleanup restores buffer sink.The destructor correctly calls
reset_default_logger()to reinstate the buffer sink, ensuring any subsequent logs are captured if the logger is used afterinit_logger_tgoes out of scope.
150-177: Race condition ininit_logger_tconstructor when called concurrently.If multiple threads invoke
init_logger_tsimultaneously (e.g., concurrentcall_solveorcall_batch_solvefrom Python/Cython), the unprotected calls tosinks().clear()andsinks().push_back()in the constructor will race, corrupting the logger's sink list or causing out-of-order message replay.Although the codebase doesn't explicitly spawn multiple threads to call
init_logger_t, the Cython interface exposes these functions to Python, where client code can invoke them concurrently. Protect the constructor body with a static mutex:init_logger_t::init_logger_t(std::string log_file, bool log_to_console) { static std::mutex init_mutex; std::lock_guard<std::mutex> lock(init_mutex); // ... rest of constructor }
|
/ok to test 78d3b97 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/tests/utilities/test_cli.cpp (1)
146-146: Consider removing diagnostic output.This debug statement prints the CLI output to stdout during test execution. While harmless, it adds noise to test output and appears to be leftover development/debugging code.
Apply this diff if you'd like to remove it:
- std::cout << "Output: " << output << std::endl;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/cuopt_cli.cpp(7 hunks)cpp/tests/utilities/test_cli.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/cuopt_cli.cpp (4)
cpp/src/utilities/logger.hpp (2)
init_logger_t(48-48)init_logger_t(50-50)cpp/src/linear_programming/solve.cu (5)
op_problem(938-938)solve_lp(791-932)solve_lp(791-795)solve_lp(1005-1014)solve_lp(1005-1010)cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
initial_solution(62-64)cpp/src/mip/solve.cu (5)
solution(82-82)solve_mip(155-296)solve_mip(155-156)solve_mip(299-306)solve_mip(299-302)
🔇 Additional comments (4)
cpp/cuopt_cli.cpp (4)
78-88: LGTM!The
dummy_loggerhelper is well-designed for early error paths. The RAII pattern ensures proper logger initialization and cleanup before error logging in edge cases.
109-111: LGTM!Consistent use of
dummy_loggerin error paths ensures logging is properly configured before errors are emitted, even when normal initialization hasn't occurred yet.Also applies to: 129-131, 165-168
138-144: Verify the logic for IP problems with relaxation.When
solve_relaxationis true for an IP (Integer Programming) problem, bothis_mipandis_lpwill be false. This causes the else branch (MIP solver) to execute at line 174, which may not be the intended behavior for a relaxation.Should
is_lpalso handle the IP + relaxation case?const bool is_lp = op_problem.get_problem_category() == cuopt::linear_programming::problem_category_t::LP || - (op_problem.get_problem_category() == cuopt::linear_programming::problem_category_t::MIP && - solve_relaxation); + ((op_problem.get_problem_category() == cuopt::linear_programming::problem_category_t::MIP || + op_problem.get_problem_category() == cuopt::linear_programming::problem_category_t::IP) && + solve_relaxation);
153-168: LGTM!Initial solution loading is properly wrapped in error handling with appropriate logger initialization before error emission.
|
/ok to test e5cc7cd |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp/cuopt_cli.cpp (1)
166-177: Consider using dummy_logger for consistency in error handling.The error logging at line 175 doesn't use
dummy_logger, unlike the error handling at lines 109, 129, and 161. If an exception escapes fromsolve_miporsolve_lp, their internalinit_logger_twill have been destroyed during stack unwinding, so line 175 would use the default logger and potentially bypass thelog_to_consolesetting.Apply this diff for consistency:
} catch (const std::exception& e) { + auto log = dummy_logger(settings); CUOPT_LOG_ERROR("Error: %s", e.what()); return -1; }Note: This is a minor consistency issue since exceptions escaping the solve functions are unexpected (they have internal error handling), but it would align with the error-handling pattern used throughout this file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp/cuopt_cli.cpp(6 hunks)cpp/src/linear_programming/solve.cu(1 hunks)cpp/src/mip/presolve/third_party_presolve.cpp(1 hunks)cpp/src/mip/solve.cu(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/linear_programming/solve.cu
- cpp/src/mip/presolve/third_party_presolve.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/cuopt_cli.cpp (3)
cpp/src/utilities/logger.hpp (2)
init_logger_t(48-48)init_logger_t(50-50)cpp/src/linear_programming/solve.cu (5)
op_problem(937-937)solve_lp(790-931)solve_lp(790-794)solve_lp(1004-1013)solve_lp(1004-1009)cpp/src/mip/solve.cu (5)
solution(83-83)solve_mip(156-302)solve_mip(156-157)solve_mip(305-312)solve_mip(305-308)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.1, 3.10, amd64, rockylinux8
🔇 Additional comments (6)
cpp/src/mip/solve.cu (1)
33-33: LGTM! Correctly addresses the logging bug.The switch from
linear_programming/utilities/logger_init.hpptoutilities/logger.hppenables the new RAII-based logger initialization at line 168, which properly configures the logger withsettings.log_to_consolebefore any log output occurs. This prevents parameter-change messages from being emitted to stdout whenlog_to_consoleis false, directly addressing the bug described in issue #187.cpp/cuopt_cli.cpp (5)
78-88: Good approach for early error logging.The
dummy_loggerhelper ensures that error paths respect thelog_to_consolesetting even before the main logger is initialized by the solver functions. The RAII pattern withinit_logger_tis appropriate here.
104-112: LGTM: Proper logger initialization for parameter errors.Using
dummy_loggerhere ensures that parameter validation errors are logged according to the configured settings before the main solver initialization.
119-132: Appropriate error handling for parsing failures.The use of
dummy_loggerat line 129 ensures parsing errors are logged with proper configuration. Note that line 120'sCUOPT_LOG_INFOcall relies on the buffered logging mechanism described in the PR (log buffer captures output until logger initialization).
137-140: Correct logic for LP relaxation handling.The addition of
!solve_relaxationproperly ensures that when solving the LP relaxation of a MIP/IP problem, the LP solver is used instead of the MIP solver.
142-164: Well-structured error handling for initial solution loading.The refactored code properly distinguishes between MIP and LP initial solution handling, and the try-catch block at lines 160-164 correctly uses
dummy_loggerto ensure error logging respects the configuration.
|
/ok to test 5da3d88 |
|
/merge |
4 participants
Description
Prior to this PR, some logging info relating to solver parameters was shown in stdout. This is because we initialize the logger to stdout and we do not have apriori information whether the corresponding parameter would be set or not. In this PR, I am using the callback logger along with a log buffer to store all the logging info until the appropriate parameters and the loggers are initialized.
Issue
Closes #187
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes / Diagnostics
Tests