Expose GPU heuristics tuning parameters via config files#993
Expose GPU heuristics tuning parameters via config files#993aliceb-nv wants to merge 8 commits intoNVIDIA:release/26.04from
Conversation
aliceb-nv
added
non-breaking
|
/ok to test cc1b185 |
📝 WalkthroughWalkthroughAdds a configurable MIP heuristic hyperparameter subsystem: new hyperparameter struct and constants, file load/dump APIs, CLI flags to show/load params, and multiple solver/heuristic components updated to consume these parameters instead of fixed constants. 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?
| #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.
| T min, | ||
| T max, | ||
| T def, | ||
| bool is_hyperparameter = false, |
There was a problem hiding this comment.
Not sure if we need is_hyperparameter are you using this anywhere? we have HYPER in the parameter names, I think we can deduce this.
| diversity_config_t make_diversity_config(const mip_heuristics_hyper_params_t& hp) | ||
| { | ||
| diversity_config_t c; | ||
| c.max_solutions = hp.population_size; |
There was a problem hiding this comment.
I feel this is error prone. Now, you will have to add new parameters at two places. How about compose the mip_heuristics_hyper_params_t class with objects like diversity_config_t and rins_settings_t directly?
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