Expose GPU heuristics tuning parameters via config files#993
Expose GPU heuristics tuning parameters via config files#993aliceb-nv wants to merge 16 commits intoNVIDIA:release/26.04from
Conversation
aliceb-nv
added
non-breaking
|
/ok to test cc1b185 |
|
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:
📝 WalkthroughWalkthroughIntroduces a MIP heuristic hyperparameter subsystem: new hyperparameter type and constants, load/dump settings file support and CLI options to dump/load params, extensive solver/heuristic code updated to read these hyperparameters, plus tests validating parsing and round-trip behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (2)
cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cuh (1)
27-28: Consider validatingcycle_lenparameter.The constructor accepts
cycle_lenwith a default of 30, which is correct. However, ifcycle_lenis 0 or negative:
curr_recent_solwould be-1(or underflow for unsigned types)- The modulo operations in
check_cycle(line 54) could exhibit undefined behavior forcycle_detection_length <= 0The hyper-parameter loading should validate this, but a defensive check here would add robustness.
🛡️ Optional: Add defensive assertion
cycle_queue_t(problem_t<i_t, f_t>& problem, i_t cycle_len = 30) - : cycle_detection_length(cycle_len), curr_recent_sol(cycle_detection_length - 1) + : cycle_detection_length(cycle_len > 0 ? cycle_len : 30), curr_recent_sol(cycle_detection_length - 1) { + cuopt_assert(cycle_len > 0, "cycle_detection_length must be positive"); for (i_t i = 0; i < cycle_detection_length; ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cuh` around lines 27 - 28, Validate the incoming cycle_len in the cycle_queue_t constructor: ensure cycle_len > 0 (or enforce a minimum like 1) before assigning it to cycle_detection_length, and adjust curr_recent_sol initialization accordingly (e.g., set curr_recent_sol = cycle_detection_length - 1 only after validation). Either assert/throw on invalid cycle_len or clamp it to a safe minimum to prevent negative/underflow values and undefined behavior in check_cycle modulo operations; update any relevant comments to reflect the defensive check.cpp/src/mip_heuristics/solve.cu (1)
274-276: Note: Duplicated presolve time limit calculation.This calculation mirrors the logic in
solver.cu(lines 115-116). While correct, the duplication could lead to drift if one location is updated without the other.Consider extracting a helper function if this pattern is used elsewhere. This is not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/solve.cu` around lines 274 - 276, The presolve_time_limit computation is duplicated; extract it into a single helper (e.g., getPresolveTimeLimit or computePresolveTimeLimit) that takes the heuristic params (accessing presolve_time_ratio and presolve_max_time) and the time_limit and returns std::min(hp.presolve_time_ratio * time_limit, hp.presolve_max_time); replace the duplicated lines in both solve.cu (the chunk using settings.heuristic_params and time_limit) and solver.cu with calls to this helper so updates remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/heuristics_hyper_params_loader.hpp`:
- Around line 133-134: The call to ::isspace in the std::find_if_not predicate
can invoke UB for signed char values; change the predicate passed to
std::find_if_not(line.begin(), line.end(), ::isspace) to a lambda that casts the
input char to unsigned char before calling std::isspace (e.g., use a predicate
like [](char ch){ return std::isspace(static_cast<unsigned char>(ch)); }), so
update the find_if_not call that sets first_non_ws to use this safe predicate.
- Around line 168-184: The function currently writes out params using the
ofstream variable `file` (and iterates `int_params` and `double_params` using
`params.*p.field`) but returns true without checking I/O status; after finishing
all writes (and flushing/closing `file`), check the stream state (e.g.,
`file.fail()`/`file.good()` or equivalent) and if any write error occurred log
using `CUOPT_LOG_ERROR` and return false, otherwise return true.
---
Nitpick comments:
In `@cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cuh`:
- Around line 27-28: Validate the incoming cycle_len in the cycle_queue_t
constructor: ensure cycle_len > 0 (or enforce a minimum like 1) before assigning
it to cycle_detection_length, and adjust curr_recent_sol initialization
accordingly (e.g., set curr_recent_sol = cycle_detection_length - 1 only after
validation). Either assert/throw on invalid cycle_len or clamp it to a safe
minimum to prevent negative/underflow values and undefined behavior in
check_cycle modulo operations; update any relevant comments to reflect the
defensive check.
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 274-276: The presolve_time_limit computation is duplicated;
extract it into a single helper (e.g., getPresolveTimeLimit or
computePresolveTimeLimit) that takes the heuristic params (accessing
presolve_time_ratio and presolve_max_time) and the time_limit and returns
std::min(hp.presolve_time_ratio * time_limit, hp.presolve_max_time); replace the
duplicated lines in both solve.cu (the chunk using settings.heuristic_params and
time_limit) and solver.cu with calls to this helper so updates remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ee3863-cc97-4a20-b7d3-216ed23a9be8
📒 Files selected for processing (17)
benchmarks/linear_programming/cuopt/run_mip.cppcpp/cuopt_cli.cppcpp/include/cuopt/linear_programming/mip/heuristics_hyper_params.hppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/recombiners/recombiner.cuhcpp/src/mip_heuristics/heuristics_hyper_params_loader.hppcpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cucpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cuhcpp/src/mip_heuristics/local_search/local_search.cucpp/src/mip_heuristics/local_search/local_search.cuhcpp/src/mip_heuristics/problem/problem.cucpp/src/mip_heuristics/problem/problem.cuhcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/tests/mip/CMakeLists.txtcpp/tests/mip/heuristics_hyper_params_test.cu
| auto first_non_ws = std::find_if_not(line.begin(), line.end(), ::isspace); | ||
| if (first_non_ws == line.end() || *first_non_ws == '#') continue; |
There was a problem hiding this comment.
Potential undefined behavior with ::isspace on signed chars.
::isspace(int) has undefined behavior when the argument is not in the range [0, UCHAR_MAX] or equal to EOF. Since char may be signed, characters with values ≥128 (extended ASCII, UTF-8 continuation bytes) become negative and trigger UB.
Proposed fix: cast to unsigned char
- auto first_non_ws = std::find_if_not(line.begin(), line.end(), ::isspace);
+ auto first_non_ws = std::find_if_not(line.begin(), line.end(),
+ [](unsigned char c) { return std::isspace(c); });📝 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.
| auto first_non_ws = std::find_if_not(line.begin(), line.end(), ::isspace); | |
| if (first_non_ws == line.end() || *first_non_ws == '#') continue; | |
| auto first_non_ws = std::find_if_not(line.begin(), line.end(), | |
| [](unsigned char c) { return std::isspace(c); }); | |
| if (first_non_ws == line.end() || *first_non_ws == '#') continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/heuristics_hyper_params_loader.hpp` around lines 133 -
134, The call to ::isspace in the std::find_if_not predicate can invoke UB for
signed char values; change the predicate passed to
std::find_if_not(line.begin(), line.end(), ::isspace) to a lambda that casts the
input char to unsigned char before calling std::isspace (e.g., use a predicate
like [](char ch){ return std::isspace(static_cast<unsigned char>(ch)); }), so
update the find_if_not call that sets first_non_ws to use this safe predicate.
| std::ofstream file(path); | ||
| if (!file.is_open()) { | ||
| CUOPT_LOG_ERROR("Cannot open file for writing: %s", path.c_str()); | ||
| return false; | ||
| } | ||
| file << "# MIP heuristic hyper-parameters (auto-generated)\n"; | ||
| file << "# Uncomment and change only the values you want to override.\n\n"; | ||
| for (const auto& p : int_params) { | ||
| file << "# " << p.description << " (int, range: [" << p.min_val << ", " << p.max_val << "])\n"; | ||
| file << "# " << p.name << " = " << params.*p.field << "\n\n"; | ||
| } | ||
| for (const auto& p : double_params) { | ||
| file << "# " << p.description << " (double, range: [" << p.min_val << ", " << p.max_val | ||
| << "])\n"; | ||
| file << "# " << p.name << " = " << params.*p.field << "\n\n"; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Missing write error check before returning success.
The function returns true without verifying that all writes succeeded. If the disk fills up or another I/O error occurs during the streaming operations, the file may be incomplete but the caller believes the dump succeeded.
Proposed fix: check stream state before returning
for (const auto& p : double_params) {
file << "# " << p.description << " (double, range: [" << p.min_val << ", " << p.max_val
<< "])\n";
file << "# " << p.name << " = " << params.*p.field << "\n\n";
}
+ if (!file) {
+ CUOPT_LOG_ERROR("Write error while dumping config to: %s", path.c_str());
+ return false;
+ }
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/heuristics_hyper_params_loader.hpp` around lines 168 -
184, The function currently writes out params using the ofstream variable `file`
(and iterates `int_params` and `double_params` using `params.*p.field`) but
returns true without checking I/O status; after finishing all writes (and
flushing/closing `file`), check the stream state (e.g.,
`file.fail()`/`file.good()` or equivalent) and if any write error occurred log
using `CUOPT_LOG_ERROR` and return false, otherwise return true.
|
Hey @aliceb-nv great to see so many parameters exposed for heuristics. What do you think about make these parameters available alongside the rest of settings in solver_settings.hpp? I think it's a bit strange to have two different methods of setting parameters (one through Python/C and command-line and the other via a config file). |
|
Is this a must have for 26.04 @aliceb-nv ? Or it can go after 26.04 as a fast follow? |
I think so too. Having a similar path for hyper parameter makes sense. With that, we can also extend support for config file. We would just read in the values and set the parameters, if it is convenient. We can label these parameters as something like CUOPT_HYPER_ to distinguish them from standard parameters. |
+1 to @rg20 's naming convention |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/src/math_optimization/solver_settings.cu (1)
519-584: Config file parsing is robust with good RAII pattern.The
std::ifstreamdestructor handles cleanup. Error messages include line context, which aids debugging. A few observations:
- The quoted-string parser only handles
\"escapes (addressed in thequote_if_neededcomment above).- Consider checking
file.bad()after the loop to detect I/O errors that occurred during reading (as opposed to EOF).Optional: add stream error check after loop
} + if (file.bad()) { + cuopt_expects(false, + error_type_t::ValidationError, + "Parameter config: I/O error reading: %s", + path.c_str()); + } CUOPT_LOG_INFO("Parameters loaded from: %s", path.c_str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/math_optimization/solver_settings.cu` around lines 519 - 584, The quoted-string parser is limited and you should also detect stream I/O errors after reading; in solver_settings_t<i_t,f_t>::load_parameters_from_file, after the getline loop completes add a check on the ifstream state (e.g., file.bad() or !file.eof() combined appropriately) and raise the same ValidationError via cuopt_expects if an I/O error occurred while reading the file so we distinguish read failures from normal EOF; keep the existing error messaging style and call-site (CUOPT_LOG_INFO) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 586-622: The function
solver_settings_t<i_t,f_t>::dump_parameters_to_file writes to an ofstream but
always returns true; after performing the writes (after the string_parameters
loop) flush the stream (file.flush()) and check the stream state (e.g.,
file.fail() or !file.good()) and return false if the stream is in a bad state so
I/O errors are propagated; on failure log an error (use CUOPT_LOG_ERROR with the
path) and otherwise return true.
- Around line 46-60: The function quote_if_needed currently only escapes double
quotes but not backslashes, causing strings like foo\" to corrupt on round-trip;
modify quote_if_needed to also escape backslashes (turn '\' into "\\") before
escaping quotes so output uses \\ for backslashes and \" for quotes, and then
update the parser logic that unescapes sequences (the code that handles escaped
characters in the string parsing routine) to recognize and correctly handle the
two-character escape sequence '\\' in addition to '\"' so both backslashes and
quotes round-trip correctly.
---
Nitpick comments:
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 519-584: The quoted-string parser is limited and you should also
detect stream I/O errors after reading; in
solver_settings_t<i_t,f_t>::load_parameters_from_file, after the getline loop
completes add a check on the ifstream state (e.g., file.bad() or !file.eof()
combined appropriately) and raise the same ValidationError via cuopt_expects if
an I/O error occurred while reading the file so we distinguish read failures
from normal EOF; keep the existing error messaging style and call-site
(CUOPT_LOG_INFO) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8b3e296-bc8d-433b-9b80-be8d95667d30
📒 Files selected for processing (7)
cpp/cuopt_cli.cppcpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/heuristics_hyper_params.hppcpp/include/cuopt/linear_programming/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/internals.hppcpp/src/math_optimization/solver_settings.cucpp/tests/mip/heuristics_hyper_params_test.cu
✅ Files skipped from review due to trivial changes (1)
- cpp/include/cuopt/linear_programming/mip/heuristics_hyper_params.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/tests/mip/heuristics_hyper_params_test.cu
- cpp/cuopt_cli.cpp
| std::string quote_if_needed(const std::string& s) | ||
| { | ||
| bool needs_quoting = s.empty() || s.find(' ') != std::string::npos || | ||
| s.find('"') != std::string::npos || s.find('\t') != std::string::npos; | ||
| if (!needs_quoting) return s; | ||
| std::string out = "\""; | ||
| for (char c : s) { | ||
| if (c == '"') | ||
| out += "\\\""; | ||
| else | ||
| out += c; | ||
| } | ||
| out += '"'; | ||
| return out; | ||
| } |
There was a problem hiding this comment.
quote_if_needed doesn't escape backslashes, risking round-trip corruption.
If a string value contains a literal backslash followed by a quote (e.g., foo\"), it will be serialized as "foo\"" but parsed back as foo" (the backslash is consumed as an escape). Consider escaping backslashes as well for a correct round-trip.
Proposed fix
std::string quote_if_needed(const std::string& s)
{
bool needs_quoting = s.empty() || s.find(' ') != std::string::npos ||
- s.find('"') != std::string::npos || s.find('\t') != std::string::npos;
+ s.find('"') != std::string::npos || s.find('\t') != std::string::npos ||
+ s.find('\\') != std::string::npos;
if (!needs_quoting) return s;
std::string out = "\"";
for (char c : s) {
- if (c == '"')
+ if (c == '\\')
+ out += "\\\\";
+ else if (c == '"')
out += "\\\"";
else
out += c;
}
out += '"';
return out;
}And update the parser (lines 554-557) to handle \\:
while (iss.get(ch)) {
- if (ch == '\\' && iss.peek() == '"') {
+ if (ch == '\\') {
iss.get(ch);
- val += '"';
+ val += ch; // handles both \" and \\
} else if (ch == '"') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/math_optimization/solver_settings.cu` around lines 46 - 60, The
function quote_if_needed currently only escapes double quotes but not
backslashes, causing strings like foo\" to corrupt on round-trip; modify
quote_if_needed to also escape backslashes (turn '\' into "\\") before escaping
quotes so output uses \\ for backslashes and \" for quotes, and then update the
parser logic that unescapes sequences (the code that handles escaped characters
in the string parsing routine) to recognize and correctly handle the
two-character escape sequence '\\' in addition to '\"' so both backslashes and
quotes round-trip correctly.
|
/ok to test c08115b |
|
You all make convincing arguments :) Agreed, the hyperparameters should indeed belong with the other params. I've modified the PR in consequence. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/src/math_optimization/solver_settings.cu (2)
532-535: Potential undefined behavior with::isspaceon signed char.
std::isspace(int)has undefined behavior when passed a value not representable asunsigned char(and notEOF). On systems wherecharis signed, extended ASCII characters (e.g., UTF-8 continuation bytes) can produce negative values.Proposed fix: cast to unsigned char
- auto first_non_ws = std::find_if_not(line.begin(), line.end(), ::isspace); + auto first_non_ws = std::find_if_not(line.begin(), line.end(), + [](unsigned char c) { return std::isspace(c); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/math_optimization/solver_settings.cu` around lines 532 - 535, The call to ::isspace inside the std::find_if_not lambda can invoke undefined behavior for signed char values; update the predicate used with std::find_if_not(line.begin(), line.end(), ::isspace) so it casts each character to unsigned char before calling std::isspace (e.g., call ::isspace(static_cast<unsigned char>(c)) or use a small lambda that does this) to ensure defined behavior when scanning lines in the code that manipulates line.begin()/line.end().
554-563: Parser only handles\"escape, not\\.This is related to the
quote_if_neededissue. The parser consumes\only when followed by", but a literal backslash in the config would be passed through unchanged. Ifquote_if_neededis updated to escape backslashes, this parser must also be updated to handle\\.Proposed fix: handle both escape sequences
while (iss.get(ch)) { - if (ch == '\\' && iss.peek() == '"') { + if (ch == '\\') { iss.get(ch); - val += '"'; + val += ch; // handles both \" and \\ } else if (ch == '"') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/math_optimization/solver_settings.cu` around lines 554 - 563, The parser loop that reads quoted strings (the while loop handling ch, as used in solver_settings parsing) currently only treats backslash when followed by a quote (consumes '\' then '"' and appends '"' ), so it fails to handle escape of a literal backslash; update that loop to recognize and consume both escape sequences: when ch == '\\' and iss.peek() == '"' append '"' (as now), and when ch == '\\' and iss.peek() == '\\' consume the second backslash and append a single backslash; otherwise keep existing behavior for closing quote and normal characters. Ensure this logic is in the same function that reads quoted values so it stays consistent with any changes to quote_if_needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 532-535: The call to ::isspace inside the std::find_if_not lambda
can invoke undefined behavior for signed char values; update the predicate used
with std::find_if_not(line.begin(), line.end(), ::isspace) so it casts each
character to unsigned char before calling std::isspace (e.g., call
::isspace(static_cast<unsigned char>(c)) or use a small lambda that does this)
to ensure defined behavior when scanning lines in the code that manipulates
line.begin()/line.end().
- Around line 554-563: The parser loop that reads quoted strings (the while loop
handling ch, as used in solver_settings parsing) currently only treats backslash
when followed by a quote (consumes '\' then '"' and appends '"' ), so it fails
to handle escape of a literal backslash; update that loop to recognize and
consume both escape sequences: when ch == '\\' and iss.peek() == '"' append '"'
(as now), and when ch == '\\' and iss.peek() == '\\' consume the second
backslash and append a single backslash; otherwise keep existing behavior for
closing quote and normal characters. Ensure this logic is in the same function
that reads quoted values so it stays consistent with any changes to
quote_if_needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8752473e-b1c5-4f83-b9dc-29546c2ceb84
📒 Files selected for processing (3)
cpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/tests/mip/heuristics_hyper_params_test.cu
✅ Files skipped from review due to trivial changes (2)
- cpp/tests/mip/heuristics_hyper_params_test.cu
- cpp/include/cuopt/linear_programming/constants.h
|
Agreed with @chris-maes that ideally all parameters should go through the same code path. Otherwise it's messy for all upstream interfaces. |
rg20
left a comment
rg20
left a comment
There was a problem hiding this comment.
Great initiative! I have some minor comments.
| T max_value; | ||
| T default_value; | ||
| bool is_hyperparameter; | ||
| const char* description; |
There was a problem hiding this comment.
I thought about adding the description for the purpose of cuopt_cli --help, however, I decided against it in favor of making the names themselves self explanatory. Further more the the macros are defined as strings, we can probably use them as descriptions. Wdyt?
There was a problem hiding this comment.
I see your point :) I was however a bit worried about the UX - some of these hyperparameters are named after internal implementation details which aren't immediately obvious or clear; this is why I thought a description field could be relevant to help guide tuning efforts (e.g. what results to expect from this "knob")
| #define CUOPT_PDLP_PRECISION "pdlp_precision" | ||
|
|
||
| /* @brief MIP heuristic hyper-parameter names */ | ||
| #define CUOPT_HYPER_POPULATION_SIZE "hyper_population_size" |
There was a problem hiding this comment.
these strings are not directly used anywhere, so you can make these as descriptive as you want and use them for descriptions.
|
/ok to test 42f977d |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
10-20:⚠️ Potential issue | 🟡 MinorDuplicate
#include <vector>detected.
<vector>is included at both line 10 and line 20. Remove one occurrence.Proposed fix
`#include` <vector> `#include` <cuopt/linear_programming/constants.h> `#include` <cuopt/linear_programming/mip/heuristics_hyper_params.hpp> `#include` <cuopt/linear_programming/pdlp/pdlp_hyper_params.cuh> `#include` <cuopt/linear_programming/utilities/internals.hpp> `#include` <raft/core/device_span.hpp> `#include` <rmm/device_uvector.hpp> - -#include <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp` around lines 10 - 20, Remove the duplicate include of the standard header by deleting one of the redundant "#include <vector>" lines in the top of the header (cuopt/linear_programming/mip/solver_settings.hpp); ensure only a single "#include <vector>" remains so the included symbols used by the file (e.g., anywhere using std::vector) are still available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/diversity/lns/rins.cu`:
- Around line 34-35: The change makes time_limit (variable time_limit in
rins.cu) derive from context.settings.heuristic_params.rins_time_limit which
defaults to 3.0 in heuristics_hyper_params.hpp, reducing the RINS sub-MIP time
limit from the previous hardcoded 10.0s; either explicitly document this tuning
decision in the code/comments (e.g., in rins.cu and heuristics_hyper_params.hpp)
stating why 3.0s is chosen, or revert the default to preserve prior behavior by
setting rins_time_limit default back to 10.0 in heuristics_hyper_params.hpp (and
ensure any comment in rins.cu reflects the chosen default) so existing behavior
remains unchanged.
---
Outside diff comments:
In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp`:
- Around line 10-20: Remove the duplicate include of the standard header by
deleting one of the redundant "#include <vector>" lines in the top of the header
(cuopt/linear_programming/mip/solver_settings.hpp); ensure only a single
"#include <vector>" remains so the included symbols used by the file (e.g.,
anywhere using std::vector) are still available.
🪄 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: Pro
Run ID: 8cbf2856-6a24-4436-9839-378438e5eb69
📒 Files selected for processing (9)
cpp/cuopt_cli.cppcpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/diversity/diversity_config.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/lns/rins.cucpp/src/mip_heuristics/diversity/lns/rins.cuhcpp/src/mip_heuristics/solve.cu
💤 Files with no reviewable changes (1)
- cpp/src/mip_heuristics/diversity/lns/rins.cuh
✅ Files skipped from review due to trivial changes (1)
- cpp/src/mip_heuristics/solve.cu
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/cuopt_cli.cpp
- cpp/src/mip_heuristics/diversity/diversity_manager.cu
- cpp/src/math_optimization/solver_settings.cu
| fixrate = context.settings.heuristic_params.rins_fix_rate; | ||
| time_limit = context.settings.heuristic_params.rins_time_limit; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the old default value and check for any documentation of this change
# Check the old default in rins.cuh
echo "=== Old default in rins.cuh ==="
rg -n 'time_limit\{' cpp/src/mip_heuristics/diversity/lns/rins.cuh
# Check the new default in heuristics_hyper_params.hpp
echo "=== New default in heuristics_hyper_params.hpp ==="
rg -n 'rins_time_limit' cpp/include/cuopt/linear_programming/mip/heuristics_hyper_params.hpp
# Check for any changelog or migration notes
echo "=== Changelog/migration notes ==="
fd -t f -i 'changelog\|migration\|release' -x head -50 {} \;Repository: NVIDIA/cuopt
Length of output: 283
Confirm intentional default change: time_limit reduced from 10.0 to 3.0 seconds.
Lines 34-35 now initialize time_limit from context.settings.heuristic_params.rins_time_limit, which defaults to 3.0 (per heuristics_hyper_params.hpp). Previously, rins.cuh hardcoded time_limit{10.}. This 70% reduction in the RINS sub-MIP time limit is a significant behavioral change with no documented justification.
Either document this as an intentional tuning decision, or align the new default to the previous value of 10.0 to preserve existing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/diversity/lns/rins.cu` around lines 34 - 35, The
change makes time_limit (variable time_limit in rins.cu) derive from
context.settings.heuristic_params.rins_time_limit which defaults to 3.0 in
heuristics_hyper_params.hpp, reducing the RINS sub-MIP time limit from the
previous hardcoded 10.0s; either explicitly document this tuning decision in the
code/comments (e.g., in rins.cu and heuristics_hyper_params.hpp) stating why
3.0s is chosen, or revert the default to preserve prior behavior by setting
rins_time_limit default back to 10.0 in heuristics_hyper_params.hpp (and ensure
any comment in rins.cu reflects the chosen default) so existing behavior remains
unchanged.
There was a problem hiding this comment.
rins.cuh did not hardcode 10. The time limit was reset to the default value in the constructor, which is 3. Can you check this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of rins.cuh for any time_limit defaults
echo "=== rins.cuh content around settings struct ==="
rg -n 'time_limit\|fixrate\|default_' cpp/src/mip_heuristics/diversity/lns/rins.cuh -A2 -B2
# Check git log for the old default values in rins.cuh
echo "=== Git diff for rins.cuh ==="
git diff HEAD~1 -- cpp/src/mip_heuristics/diversity/lns/rins.cuh 2>/dev/null || git log --oneline -5
Length of output: 806
@aliceb-nv, you're absolutely right. The git diff confirms that the old default_time_limit in rins_settings_t was indeed 3.0:
- double default_time_limit = 3.;
The new rins_time_limit default of 3.0 in heuristics_hyper_params.hpp exactly matches the previous value — there is no behavioral change here. My original comment was incorrect. Apologies for the noise!
|
Thinking a bit about naming (there are only two hard things in computer science: naming and cache invalidation). We've tried to use the prefix "mip" for all MIP setting names. What about calling these ``` if that is too wordy you could also do We could use the convention that if a parameter has the name "hyper" in it we don't print it out in Also, does your settings file work for all settings? Not just the hyper ones? If so, that is a useful feature. |
|
/ok to test 7788f3b |
|
Good points :) I ended up settling on the Yes, the parameter config file loading works with all parameters! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/cuopt_cli.cpp`:
- Around line 93-100: The per-device RMM setup happens in main() before
run_single_file() loads params_file, so a config-only CUOPT_NUM_GPUS in
params_file is ignored; fix by merging file and CLI settings before RMM/GPU
initialization: either pre-read params_file (call
settings.load_parameters_from_file(params_file) or a lightweight parser) in
main() to extract CUOPT_NUM_GPUS and override/merge with settings_strings, or
change run_single_file()/its caller to accept a fully merged solver_settings_t
(created by loading params_file then applying settings_strings) and use that
merged settings to drive the RMM/device setup; ensure references to params_file,
settings.load_parameters_from_file, run_single_file, main, and CUOPT_NUM_GPUS
are updated accordingly.
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 13-16: The parsing code currently uses std::stof/std::stod and
then only performs range checks, but "nan" parses successfully and escapes those
checks; update the parser wherever std::stof or std::stod is used (the
float/double parsing sites in this file) to detect and reject NaN results by
calling std::isnan(parsedValue) immediately after parsing and treating it as an
invalid config value (log/return error/throw as the surrounding code does for
other parse failures); do this for both float and double parsing sites mentioned
(the blocks using std::stof and std::stod) so malformed "nan" strings cannot
propagate into solver settings.
In `@cpp/src/mip_heuristics/problem/problem.cuh`:
- Line 331: The three copy constructors of the Problem class in problem.cuh are
not propagating the tuned related_vars_time_limit, causing copies (e.g., via
get_problem_after_fixing_vars) to reset it to the default; update each copy
constructor initializer list to include
related_vars_time_limit(problem_.related_vars_time_limit) so the copied Problem
preserves the source's related_vars_time_limit value.
🪄 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: Pro
Run ID: 4f104d6c-54e1-4ffe-a4c1-5e3c5f0c2416
📒 Files selected for processing (9)
cpp/cuopt_cli.cppcpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/utilities/internals.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/problem/problem.cucpp/src/mip_heuristics/problem/problem.cuhcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/tests/mip/heuristics_hyper_params_test.cu
✅ Files skipped from review due to trivial changes (3)
- cpp/src/mip_heuristics/problem/problem.cu
- cpp/tests/mip/heuristics_hyper_params_test.cu
- cpp/include/cuopt/linear_programming/constants.h
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/solve.cu
cpp/cuopt_cli.cpp
Outdated
| const std::map<std::string, std::string>& settings_strings, | ||
| const std::string& params_file = "") | ||
| { | ||
| cuopt::linear_programming::solver_settings_t<int, double> settings; | ||
|
|
||
| try { | ||
| if (!params_file.empty()) { settings.load_parameters_from_file(params_file); } | ||
| for (auto& [key, val] : settings_strings) { |
There was a problem hiding this comment.
--params-file is loaded too late for CUOPT_NUM_GPUS.
main() configures per-device RMM resources before run_single_file() loads the file, so a config-only num_gpus setting is ignored during GPU resource setup. Please merge file + CLI settings before that initialization step, or pre-read at least CUOPT_NUM_GPUS from the file.
Also applies to: 419-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/cuopt_cli.cpp` around lines 93 - 100, The per-device RMM setup happens in
main() before run_single_file() loads params_file, so a config-only
CUOPT_NUM_GPUS in params_file is ignored; fix by merging file and CLI settings
before RMM/GPU initialization: either pre-read params_file (call
settings.load_parameters_from_file(params_file) or a lightweight parser) in
main() to extract CUOPT_NUM_GPUS and override/merge with settings_strings, or
change run_single_file()/its caller to accept a fully merged solver_settings_t
(created by loading params_file then applying settings_strings) and use that
merged settings to drive the RMM/device setup; ensure references to params_file,
settings.load_parameters_from_file, run_single_file, main, and CUOPT_NUM_GPUS
are updated accordingly.
|
/ok to test 41a32f3 |
5 participants
This PR exposes some GPU heuristics tuning parameters to the user through a configuration file of the following format:
Parameters are only exposed through the cuopt_cli runner for the moment, via the flags
--dump-mip-heuristic-config(to export a sample config file with the default parameters), and--mip-heuristic-config(to load the parameters from a file).Description
Issue
Checklist