Add new solver setting CUOPT_MIP_PROBING and separate it from presolve#1257
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesProbing Parameter Addition
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/grpc/cuopt_remote.proto`:
- Around line 204-205: The plain proto3 bool field `probing` cannot express
presence so older clients default it to false; change the proto declaration for
`probing` to be presence-aware (use proto3 "optional bool probing = 29;" or a
google.protobuf.BoolValue wrapper) and then modify grpc_settings_mapper.cpp to
only assign into `settings.probing` when the field is present by checking
`pb_settings.has_probing()` before doing the assignment (i.e., call
`pb_settings.probing()` only inside the presence check) so older clients
preserve the intended default-true behavior.
🪄 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: aa68fac5-8885-42e6-86cb-df6679d32bb0
📒 Files selected for processing (9)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/grpc/cuopt_remote.protocpp/src/grpc/grpc_settings_mapper.cppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/diversity/diversity_manager.cudocs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rstdocs/cuopt/source/lp-qp-milp-settings.rstpython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py
chris-maes
left a comment
There was a problem hiding this comment.
CUOPT_PROBING -> CUOPT_MIP_PROBING
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/math_optimization/solver_settings.cu (1)
177-177: ⚡ Quick winAdd an inline description for the new probing parameter.
CUOPT_MIP_PROBINGis an algorithm parameter, but this registration omits the description field that your parameter dump/help path uses.As per coding guidelines, "Flag missing documentation for numerical tolerances and algorithm parameters".Suggested patch
- {CUOPT_MIP_PROBING, &mip_settings.probing, true}, + {CUOPT_MIP_PROBING, + &mip_settings.probing, + true, + "enable probing-cache step in cuOpt internal MIP presolve (ignored when presolve is off)"},🤖 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/math_optimization/solver_settings.cu` at line 177, The registration entry for the new algorithm parameter CUOPT_MIP_PROBING (which references mip_settings.probing) is missing the human-readable description used by the parameter dump/help path; update the registration tuple/struct where CUOPT_MIP_PROBING is added so it includes an appropriate inline description string (briefly describing what probing does and valid values/default behavior) alongside the existing name, pointer, and enabled flag so the help/dump output documents the parameter correctly.
🤖 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.
Nitpick comments:
In `@cpp/src/math_optimization/solver_settings.cu`:
- Line 177: The registration entry for the new algorithm parameter
CUOPT_MIP_PROBING (which references mip_settings.probing) is missing the
human-readable description used by the parameter dump/help path; update the
registration tuple/struct where CUOPT_MIP_PROBING is added so it includes an
appropriate inline description string (briefly describing what probing does and
valid values/default behavior) alongside the existing name, pointer, and enabled
flag so the help/dump output documents the parameter correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2844f9f5-8c3e-4cb0-9899-214b920ce69e
📒 Files selected for processing (6)
cpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/diversity/diversity_manager.cudocs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rstdocs/cuopt/source/lp-qp-milp-settings.rstpython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py
chris-maes
left a comment
There was a problem hiding this comment.
LGTM. Thanks for adding this setting @akifcorduk
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Approving all other changes, not completely sure about GRPC part, but mostly looks good.
|
/merge |
Conflicts resolved: - build.sh: keep 'codegen' build target (PR), drop libmps_parser/cuopt_mps_parser build blocks (removed on release/26.06). - cpp/src/grpc/cuopt_remote.proto: keep PR's slimmed-down service-only file that imports cuopt_remote_data.proto. - cpp/src/grpc/grpc_settings_mapper.cpp: keep PR's generated #include version. To preserve the new 'bool probing' MIP setting introduced on release/26.06 (NVIDIA#1257), it was added to the codegen registry. Because the PR had already claimed proto field 29 for mip_batch_pdlp_reliability_branching, that field was renumbered to 32 and probing now occupies field 29 (matching the wire layout already shipped on release/26.06). Generated .inc files and cuopt_remote_data.proto were re-generated from field_registry.yaml.
Add CUOPT_MIP_PROBING setting (default true) to let users disable the probing-cache step of cuOpt's internal MIP presolve without disabling the rest of presolve. Existing CUOPT_PRESOLVE=0 still turns the whole pipeline off, so CUOPT_MIP_PROBING only matters when presolve is otherwise enabled.