MIP log cleanup#1402
Open
nguidotti wants to merge 27 commits into
Open
Conversation
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…an method. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…ogs to use the std::format variant. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…ly with restarts. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a B&B timing field to solver stats, refactors branch-and-bound progress/table reporting with unified gap formatting and header helper, introduces std::format-based logger helpers, makes solution-summary logging multi-line and unconditional, and applies minor logging message/level tweaks across presolve/cuts/diversity code. ChangesMIP Solver Reporting and Diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (2)
cpp/src/mip_heuristics/solver.cu (1)
493-496: ⚡ Quick winUse
steady_clockfor elapsed B&B timing.
std::chrono::system_clockcan jump if wall time changes, which can produce inaccuratebnb_time. Usestd::chrono::steady_clockfor duration measurement.Suggested change
- auto t0 = std::chrono::system_clock::now(); + auto t0 = std::chrono::steady_clock::now(); branch_and_bound_status = branch_and_bound->solve(branch_and_bound_solution); - std::chrono::duration<double> elapsed = std::chrono::system_clock::now() - t0; + std::chrono::duration<double> elapsed = std::chrono::steady_clock::now() - t0; context.stats.bnb_time = elapsed.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 `@cpp/src/mip_heuristics/solver.cu` around lines 493 - 496, The timing uses std::chrono::system_clock which can jump; change it to use std::chrono::steady_clock for measuring branch-and-bound elapsed time: replace the t0 capture and subsequent duration calculation around branch_and_bound->solve(branch_and_bound_solution) so that t0 is set with std::chrono::steady_clock::now() and the duration is computed from std::chrono::steady_clock::now() - t0, then store elapsed.count() into context.stats.bnb_time (leaving branch_and_bound_status and branch_and_bound->solve unchanged).cpp/src/branch_and_bound/branch_and_bound.cpp (1)
37-43: ⚡ Quick winAdd the direct
<format>include forstd::formatusage.This file now relies on
std::formatin multiple paths, but it does not include<format>directly. Please add it to avoid transitive-include fragility.Suggested diff
`#include` <algorithm> `#include` <cmath> `#include` <cstdio> `#include` <cstdlib> +#include <format> `#include` <limits> `#include` <string> `#include` <vector>As per coding guidelines, “Apply Include What You Use (IWYU) principle in C++ headers.”
Also applies to: 211-217, 335-357
🤖 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/branch_and_bound/branch_and_bound.cpp` around lines 37 - 43, Summary: The file uses std::format but doesn't directly include <format>, violating IWYU; add the direct include. Update branch_and_bound.cpp by inserting `#include` <format> alongside the other headers near the top so std::format calls compile reliably; also scan the locations mentioned (lines around the blocks that use std::format—the contexts referenced as 211-217 and 335-357) and add or ensure a <format> include there or at the top of the translation unit if those uses are in the same file, guaranteeing all std::format uses (e.g., in functions that call std::format) have the direct <format> include.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.
Inline comments:
In `@cpp/src/dual_simplex/logger.hpp`:
- Around line 19-20: The header cpp/src/dual_simplex/logger.hpp uses
std::string_view (referenced by the CUOPT_LOG_ACTIVE_LEVEL macro and related
logging functions) but doesn't include <string_view>; add `#include` <string_view>
alongside the other includes at the top of logger.hpp so the header is
self-contained and no longer relies on transitive includes.
- Around line 96-97: The logger misuses std::string_view (calling c_str()) and
creates a view into a temporary, and print_format ignores the trimmed view; fix
by producing an owning trimmed std::string when modification is needed and use
its c_str() for logging, and update both debug_format and print_format to use
that trimmed std::string (or, if you prefer non-owning, use msg_view.data() with
a length-based logging API) so no dangling pointer or ignored trimming occurs;
change references in logger.hpp in the debug_format and print_format
implementations to use the corrected trimmed std::string (or data()+length
approach) instead of msg_view.c_str() or msg.c_str().
In `@cpp/src/mip_heuristics/solver_solution.cu`:
- Around line 252-256: The current branch for !has_solution sets obj, gap, and
dual_bound to infinity which overwrites a valid solver bound; modify the block
so that when has_solution is false you still set obj and gap to
std::numeric_limits<f_t>::infinity() but do not overwrite dual_bound — either
leave dual_bound as obtained from stats_.get_solution_bound() or explicitly set
dual_bound = stats_.get_solution_bound(); locate the logic around has_solution,
obj, gap, dual_bound in solver_solution.cu and update accordingly.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 37-43: Summary: The file uses std::format but doesn't directly
include <format>, violating IWYU; add the direct include. Update
branch_and_bound.cpp by inserting `#include` <format> alongside the other headers
near the top so std::format calls compile reliably; also scan the locations
mentioned (lines around the blocks that use std::format—the contexts referenced
as 211-217 and 335-357) and add or ensure a <format> include there or at the top
of the translation unit if those uses are in the same file, guaranteeing all
std::format uses (e.g., in functions that call std::format) have the direct
<format> include.
In `@cpp/src/mip_heuristics/solver.cu`:
- Around line 493-496: The timing uses std::chrono::system_clock which can jump;
change it to use std::chrono::steady_clock for measuring branch-and-bound
elapsed time: replace the t0 capture and subsequent duration calculation around
branch_and_bound->solve(branch_and_bound_solution) so that t0 is set with
std::chrono::steady_clock::now() and the duration is computed from
std::chrono::steady_clock::now() - t0, then store elapsed.count() into
context.stats.bnb_time (leaving branch_and_bound_status and
branch_and_bound->solve unchanged).
🪄 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: 4c889dc2-982e-450d-8474-f1199d65f7fb
📒 Files selected for processing (7)
cpp/include/cuopt/linear_programming/mip/solver_stats.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/logger.hppcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/src/mip_heuristics/solver_solution.cu
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
commented
Jun 9, 2026
| papilo_problem.getNRows(), | ||
| papilo_problem.getNCols(), | ||
| papilo_problem.getConstraintMatrix().getNnz()); | ||
| CUOPT_LOG_DEBUG("Original problem: %d constraints, %d variables, %d nonzeros", |
Contributor
Author
There was a problem hiding this comment.
We already log the dimensions of the problems in another place. This is duplicated info
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/dual_simplex/logger.hpp (1)
97-100: ⚡ Quick winFix inconsistent newline check and consider more efficient trimming.
Lines 97–98 (and 154–155 in
debug_format) checkmsg.ends_with("\n")but then modifymsg_no_newline. This inconsistency is confusing and could cause bugs if the code is later refactored. Additionally, copying the entire string just to remove one character is inefficient.♻️ Suggested improvement
Use
string_viewto avoid the copy, or at least check the same variable you modify:`#ifdef` CUOPT_LOG_ACTIVE_LEVEL - std::string msg_no_newline = msg; - if (msg_no_newline.size() > 0 && msg.ends_with("\n")) { msg_no_newline.pop_back(); } - - CUOPT_LOG_INFO("%s%s", log_prefix.c_str(), msg_no_newline.c_str()); + std::string_view msg_view{msg}; + if (!msg_view.empty() && msg_view.back() == '\n') { msg_view.remove_suffix(1); } + CUOPT_LOG_INFO("%s%.*s", log_prefix.c_str(), static_cast<int>(msg_view.size()), msg_view.data());Apply the analogous change in
debug_format(lines 154–156), replacingCUOPT_LOG_INFOwithCUOPT_LOG_TRACE.Also applies to: 154-156
🤖 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/dual_simplex/logger.hpp` around lines 97 - 100, The newline-trim is inconsistent and inefficient: the code checks msg.ends_with("\n") but pops from msg_no_newline and copies the whole string; change both occurrences (in the function that logs with CUOPT_LOG_INFO and in debug_format which should use CUOPT_LOG_TRACE) to operate on a std::string_view (or at minimum check and pop from the same variable) so you avoid the full copy — e.g., create a std::string_view sv = msg; if (sv.size() && sv.back() == '\n') sv.remove_suffix(1); then pass sv.data() (or format from the view) to CUOPT_LOG_INFO and use CUOPT_LOG_TRACE in the debug_format variant.
🤖 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/dual_simplex/logger.hpp`:
- Line 94: Replace the non-idiomatic perfect-forwarding usage
std::forward<Args&&>(args)... with std::forward<Args>(args)... in both places
where the formatted message is constructed: the std::format call that
initializes msg and the debug_format function; ensure you update the template
forwarding in those call sites (refer to the msg construction and debug_format)
to use std::forward<Args>(args)... so forwarding follows standard C++
conventions.
---
Nitpick comments:
In `@cpp/src/dual_simplex/logger.hpp`:
- Around line 97-100: The newline-trim is inconsistent and inefficient: the code
checks msg.ends_with("\n") but pops from msg_no_newline and copies the whole
string; change both occurrences (in the function that logs with CUOPT_LOG_INFO
and in debug_format which should use CUOPT_LOG_TRACE) to operate on a
std::string_view (or at minimum check and pop from the same variable) so you
avoid the full copy — e.g., create a std::string_view sv = msg; if (sv.size() &&
sv.back() == '\n') sv.remove_suffix(1); then pass sv.data() (or format from the
view) to CUOPT_LOG_INFO and use CUOPT_LOG_TRACE in the debug_format variant.
🪄 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: 8d7a3265-92d5-4334-8cb3-00dcf9c3dc2e
📒 Files selected for processing (1)
cpp/src/dual_simplex/logger.hpp
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Contributor
|
Is there an easy way to show a side-by-side of the key differences before and after to facilitate review? |
Contributor
Author
I can add diff of each log |
Contributor
Author
|
"old" logs (from the main branch): Side-by-side: edit: the spacing in the diff version make it harder to see. I think the best way is to open them side-by-side in your text editor |
The MIP log cleanup (PR NVIDIA#1402) renamed `log_detailed_summary()` output from "Solution objective: ..." to "Best objective ...", but test_cli.sh was not updated to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
requested review from
chris-maes and
mlubin
and removed request for
yuwenchen95
Contributor
Contributor
|
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
2 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.
This PR cleans the logs for the MIP solver. More specifically,
std::formatstd::formatto thelogger_tExamples
feasible.log
infeasible.log
optimal.log
timeout.log
Checklist