Conditional build for remote execution - #1128
Merged
Merged
Conversation
…ved DISABLE_OPENMP flag since it always needed by the MIP solver. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
added
non-breaking
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
build.sh (1)
241-259:⚠️ Potential issue | 🟠 MajorReject
cuopt_grpc_serverwhen gRPC build support is disabled.
-tsannow forcesSKIP_GRPC_BUILD=1, but Line 397 still allowscuopt_grpc_serverto be requested. That turns into a late unknown-target failure instead of a clear CLI validation error.Suggested fix
if hasArg --skip-grpc-build; then SKIP_GRPC_BUILD=1 fi + +if [ ${SKIP_GRPC_BUILD} -eq 1 ] && hasArg cuopt_grpc_server; then + echo "ERROR: cuopt_grpc_server requires gRPC build support; remove --skip-grpc-build/-tsan" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.sh` around lines 241 - 259, When BUILD_TSAN sets SKIP_GRPC_BUILD=1 we must reject any request for the cuopt_grpc_server target early; update the CLI validation that currently accepts targets (the block that checks requested build targets / the logic that later errors on unknown targets) to explicitly fail if SKIP_GRPC_BUILD=1 and the target list includes "cuopt_grpc_server". Locate the target-parsing/validation code (the section that examines requested targets/hasArg and currently allows cuopt_grpc_server around the later check) and add a check that prints a clear validation error and exits when SKIP_GRPC_BUILD=1 and "cuopt_grpc_server" is present. Ensure the message references SKIP_GRPC_BUILD/BUILD_TSAN and the cuopt_grpc_server target so users get a clear CLI error instead of a late unknown-target failure.cpp/src/pdlp/solve.cu (1)
1745-1757:⚠️ Potential issue | 🟠 MajorAdd validation when remote execution is requested in a gRPC-less build.
When
CUOPT_ENABLE_GRPCis undefined, the remote execution check is skipped entirely. However,is_remote_execution_enabled()still returns true if the environment variables (CUOPT_REMOTE_HOSTandCUOPT_REMOTE_PORT) are set, causing silent fallback to local execution instead of surfacing that gRPC support was compiled out. This masks deployment misconfiguration.Suggested fix
`#ifdef` CUOPT_ENABLE_GRPC if (is_remote_execution_enabled()) { cuopt_expects(!is_batch_mode, error_type_t::ValidationError, "Batch mode with remote execution is not supported via this entry point. " "Use solve_batch_remote() instead."); auto* cpu_prob = dynamic_cast<cpu_optimization_problem_t<i_t, f_t>*>(problem_interface); cuopt_expects(cpu_prob != nullptr, error_type_t::ValidationError, "Remote execution requires CPU memory backend"); return solve_lp_remote(*cpu_prob, settings); } +#else + cuopt_expects(!is_remote_execution_enabled(), + error_type_t::ValidationError, + "Remote execution was requested, but this build was compiled without gRPC support"); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/pdlp/solve.cu` around lines 1745 - 1757, Currently when CUOPT_ENABLE_GRPC is not defined the code skips the remote-execution branch and silently proceeds with local solve even if is_remote_execution_enabled() returns true; add an explicit validation guard in the surrounding function so that when is_remote_execution_enabled() is true but CUOPT_ENABLE_GRPC is not defined you call cuopt_expects(...) (or similar error path) to raise an error_type_t::ValidationError with a clear message indicating gRPC support was not compiled in and suggesting to enable CUOPT_ENABLE_GRPC or unset CUOPT_REMOTE_HOST/CUOPT_REMOTE_PORT; reference the existing symbols is_remote_execution_enabled(), CUOPT_ENABLE_GRPC, solve_lp_remote and solve_batch_remote so the check is colocated where the current `#ifdef` CUOPT_ENABLE_GRPC remote block lives.
🤖 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/CMakeLists.txt`:
- Around line 198-202: The CMake config currently calls find_package(OpenMP)
without REQUIRED while later unconditionally linking OpenMP::OpenMP_CXX (used in
CUOPT_PRIVATE_CUDA_LIBS and various targets); change the call to require OpenMP
at configure time by updating the find_package invocation to request REQUIRED
(so configuration fails with a clear message if absent) and remove the
now-redundant OPENMP_FOUND conditional message or keep it for verbosity — ensure
references to OpenMP::OpenMP_CXX and CUOPT_PRIVATE_CUDA_LIBS remain intact.
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 627-635: The code currently skips the remote-execution check when
CUOPT_ENABLE_GRPC is not defined, allowing a silent fall-through to local
execution; change the block so that is_remote_execution_enabled() is always
checked at runtime and, if true but CUOPT_ENABLE_GRPC is not defined, return a
validation error instead of dispatching locally. Concretely, keep the existing
branch that calls solve_mip_remote(*cpu_prob, settings) when CUOPT_ENABLE_GRPC
is defined, and add an `#else` (or a separate runtime branch) that detects
is_remote_execution_enabled() and returns a failure (using cuopt_expects or the
same error_type_t::ValidationError pattern) with a clear message that
gRPC/remote execution was requested but the binary was built without
CUOPT_ENABLE_GRPC; reference is_remote_execution_enabled(), CUOPT_ENABLE_GRPC,
solve_mip_remote, and cpu_optimization_problem_t in the change.
---
Outside diff comments:
In `@build.sh`:
- Around line 241-259: When BUILD_TSAN sets SKIP_GRPC_BUILD=1 we must reject any
request for the cuopt_grpc_server target early; update the CLI validation that
currently accepts targets (the block that checks requested build targets / the
logic that later errors on unknown targets) to explicitly fail if
SKIP_GRPC_BUILD=1 and the target list includes "cuopt_grpc_server". Locate the
target-parsing/validation code (the section that examines requested
targets/hasArg and currently allows cuopt_grpc_server around the later check)
and add a check that prints a clear validation error and exits when
SKIP_GRPC_BUILD=1 and "cuopt_grpc_server" is present. Ensure the message
references SKIP_GRPC_BUILD/BUILD_TSAN and the cuopt_grpc_server target so users
get a clear CLI error instead of a late unknown-target failure.
In `@cpp/src/pdlp/solve.cu`:
- Around line 1745-1757: Currently when CUOPT_ENABLE_GRPC is not defined the
code skips the remote-execution branch and silently proceeds with local solve
even if is_remote_execution_enabled() returns true; add an explicit validation
guard in the surrounding function so that when is_remote_execution_enabled() is
true but CUOPT_ENABLE_GRPC is not defined you call cuopt_expects(...) (or
similar error path) to raise an error_type_t::ValidationError with a clear
message indicating gRPC support was not compiled in and suggesting to enable
CUOPT_ENABLE_GRPC or unset CUOPT_REMOTE_HOST/CUOPT_REMOTE_PORT; reference the
existing symbols is_remote_execution_enabled(), CUOPT_ENABLE_GRPC,
solve_lp_remote and solve_batch_remote so the check is colocated where the
current `#ifdef` CUOPT_ENABLE_GRPC remote block lives.
🪄 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 Plus
Run ID: ae6ecc25-4b89-4471-b536-ace97ceac5fa
📒 Files selected for processing (4)
build.shcpp/CMakeLists.txtcpp/src/mip_heuristics/solve.cucpp/src/pdlp/solve.cu
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
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/CMakeLists.txt (1)
49-60:⚠️ Potential issue | 🟡 MinorRemove the stale
DISABLE_OPENMPlog line.Line 56 still references
${DISABLE_OPENMP}after that option was removed, so verbose configure output now prints a misleading empty value.Suggested cleanup
-message(VERBOSE "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}") +message(VERBOSE "cuOpt: OpenMP required: ON")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 49 - 60, The verbose configure message still references the removed CMake variable ${DISABLE_OPENMP}; update the CMakeLists.txt by removing or replacing the message line that prints "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}" so it no longer emits a stale/empty value—locate the message(...) call that includes ${DISABLE_OPENMP} and either delete that message or change it to a valid current variable (or a static message) to keep configure output accurate.
🤖 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/CMakeLists.txt`:
- Around line 363-374: The custom add_custom_command that generates
GRPC_PROTO_SRCS/GRPC_PROTO_HDRS/GRPC_SERVICE_SRCS/GRPC_SERVICE_HDRS must also
declare a dependency on the generated base protobuf headers so service
compilation cannot race; update the add_custom_command that uses
GRPC_PROTO_FILE/PROTO_FILE (for cuopt_remote_service.proto) to add the generated
base header (e.g. the cuopt_remote.pb.h target or its generated path) to DEPENDS
so that cuopt_remote_service.pb.cc cannot be built before cuopt_remote.pb.h is
produced.
---
Outside diff comments:
In `@cpp/CMakeLists.txt`:
- Around line 49-60: The verbose configure message still references the removed
CMake variable ${DISABLE_OPENMP}; update the CMakeLists.txt by removing or
replacing the message line that prints "cuOpt: Disable OpenMP:
${DISABLE_OPENMP}" so it no longer emits a stale/empty value—locate the
message(...) call that includes ${DISABLE_OPENMP} and either delete that message
or change it to a valid current variable (or a static message) to keep configure
output accurate.
🪄 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 Plus
Run ID: 4a22deaa-314f-4c2b-9d7d-c29b37800826
📒 Files selected for processing (3)
cpp/CMakeLists.txtcpp/src/mip_heuristics/solve.cucpp/src/pdlp/solve.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/pdlp/solve.cu
tmckayus
approved these changes
Apr 21, 2026
ramakrishnap-nv
requested changes
Apr 21, 2026
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/CMakeLists.txt (1)
56-56: Drop the staleDISABLE_OPENMPlog.Line 56 still logs a removed option, so verbose configure output now shows an empty/misleading value. Consider replacing it with the new gRPC flag.
Suggested cleanup
-message(VERBOSE "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}") +message(VERBOSE "cuOpt: Skip gRPC build: ${SKIP_GRPC_BUILD}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` at line 56, The CMakeLists line currently logs a removed option via message(VERBOSE "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}"); remove or replace this stale log so configure output isn't misleading by either deleting the message or changing it to report the repository's current gRPC flag instead (e.g., reference the actual gRPC CMake variable used in the project such as GRPC_ENABLED or DISABLE_GRPC) and update the text to something like "cuOpt: gRPC: <flag_variable>" for clear verbose output.
🤖 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/CMakeLists.txt`:
- Around line 312-338: Replace the fragile get_target_property lookups with
generator-expression and target-existence checks: for protoc, test TARGET
protobuf::protoc then TARGET protoc (in-tree) and set _PROTOBUF_PROTOC to
"$<TARGET_FILE:protobuf::protoc>" or "$<TARGET_FILE:protoc>" respectively, and
only then fall back to find_program() to populate _PROTOBUF_PROTOC; for the
plugin, check TARGET gRPC::grpc_cpp_plugin first, then TARGET grpc_cpp_plugin,
setting _GRPC_CPP_PLUGIN_EXECUTABLE to "$<TARGET_FILE:gRPC::grpc_cpp_plugin>" or
"$<TARGET_FILE:grpc_cpp_plugin>" respectively, and otherwise use find_program()
as the final fallback, preserving the existing FATAL_ERROR if either
_PROTOBUF_PROTOC or _GRPC_CPP_PLUGIN_EXECUTABLE is still empty.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Line 56: The CMakeLists line currently logs a removed option via
message(VERBOSE "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}"); remove or replace
this stale log so configure output isn't misleading by either deleting the
message or changing it to report the repository's current gRPC flag instead
(e.g., reference the actual gRPC CMake variable used in the project such as
GRPC_ENABLED or DISABLE_GRPC) and update the text to something like "cuOpt:
gRPC: <flag_variable>" for clear verbose output.
🪄 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 Plus
Run ID: 15ab9adf-2617-46d8-ab8d-2dd8afbd37a7
📒 Files selected for processing (1)
cpp/CMakeLists.txt
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/CMakeLists.txt (1)
49-60:⚠️ Potential issue | 🟡 MinorUpdate the configure log for the new gRPC option.
DISABLE_OPENMPis no longer defined, so Line 56 now prints an empty/stale setting while the newSKIP_GRPC_BUILDoption is not logged. That makes configure output misleading when debugging this flag.Suggested fix
option(SKIP_GRPC_BUILD "Skip building gRPC and protobuf components" OFF) @@ -message(VERBOSE "cuOpt: Disable OpenMP: ${DISABLE_OPENMP}") +message(VERBOSE "cuOpt: Skip gRPC build: ${SKIP_GRPC_BUILD}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 49 - 60, The configure output prints a stale DISABLE_OPENMP variable and omits the new SKIP_GRPC_BUILD flag; update the message block to remove or replace the reference to DISABLE_OPENMP and add a new verbose message for SKIP_GRPC_BUILD so the configure log shows the gRPC option state. Specifically, edit the message(...) lines around the existing messages (the one currently using DISABLE_OPENMP and the other cuOpt messages) to remove the bad DISABLE_OPENMP reference and add a line like a verbose message for SKIP_GRPC_BUILD (referencing the SKIP_GRPC_BUILD option) so the configure output accurately reflects the new option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/CMakeLists.txt`:
- Around line 49-60: The configure output prints a stale DISABLE_OPENMP variable
and omits the new SKIP_GRPC_BUILD flag; update the message block to remove or
replace the reference to DISABLE_OPENMP and add a new verbose message for
SKIP_GRPC_BUILD so the configure log shows the gRPC option state. Specifically,
edit the message(...) lines around the existing messages (the one currently
using DISABLE_OPENMP and the other cuOpt messages) to remove the bad
DISABLE_OPENMP reference and add a line like a verbose message for
SKIP_GRPC_BUILD (referencing the SKIP_GRPC_BUILD option) so the configure output
accurately reflects the new option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7988908-cbc7-4789-a634-8a17786f79aa
📒 Files selected for processing (1)
cpp/CMakeLists.txt
ramakrishnap-nv
approved these changes
Apr 22, 2026
rg20
approved these changes
Apr 22, 2026
Contributor
Author
|
/merge |
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
4 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.
Added flag in
CMakeList.txtandbuild.shto conditionally build the remote execution engine. This also makes OpenMP required for compilation.Checklist