Add cuopt_grpc_server as an option for the cuopt image#1003
Open
tmckayus wants to merge 5 commits intoNVIDIA:release/26.04from
Open
Add cuopt_grpc_server as an option for the cuopt image#1003tmckayus wants to merge 5 commits intoNVIDIA:release/26.04from
tmckayus wants to merge 5 commits intoNVIDIA:release/26.04from
Conversation
tmckayus
added
non-breaking
Contributor
Author
|
Will also add docs to the PR .... |
|
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:
📝 WalkthroughWalkthroughThis pull request introduces gRPC remote execution support to NVIDIA cuOpt by updating container configuration, Docker entrypoint logic, server/client default ports from 8765 to 5001, and adding comprehensive gRPC documentation while removing the legacy GRPC_INTERFACE.md. Install selector UI gains Docker registry selection (Hub vs NGC). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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: 1
🧹 Nitpick comments (1)
ci/docker/entrypoint.sh (1)
17-17: Consider addingset -uto catch unbound variable typos.Per repository conventions, Bash scripts should use
set -uto detect unbound variables. If added, you'll need to adjust line 22 to handle the case whereCUOPT_SERVER_TYPEis intentionally unset:-set -e +set -eu-if [ "${CUOPT_SERVER_TYPE}" = "grpc" ]; then +if [ "${CUOPT_SERVER_TYPE:-}" = "grpc" ]; thenBased on learnings: "In this repository, prefer using 'set -u' in Bash scripts to detect unbound variables."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/docker/entrypoint.sh` at line 17, Add `set -u` alongside the existing `set -e` to enable unbound-variable checks, and update any direct references to CUOPT_SERVER_TYPE (the variable checked later) to avoid failing when it is intentionally unset — for example, use parameter expansion like `${CUOPT_SERVER_TYPE:-}` in comparisons or conditionals, or explicitly test for set/unset with `[ -z "${CUOPT_SERVER_TYPE+x}" ]` before using it; update the code paths that currently assume CUOPT_SERVER_TYPE exists (the conditional that handles server type) to use one of these safe patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/docker/entrypoint.sh`:
- Around line 32-35: The current block that reads CUOPT_GRPC_ARGS into EXTRA via
read -ra will split on whitespace and lose embedded spaces in quoted arguments;
either document this limitation or change parsing to preserve quoted
segments—replace the read -ra approach with a safe eval-based split (e.g., eval
"set -- $CUOPT_GRPC_ARGS" then append "$@" to GRPC_CMD) so quoted values like
"--arg \"value with spaces\"" are preserved; update the comment above the block
to mention CUOPT_GRPC_ARGS semantics and reference CUOPT_GRPC_ARGS and GRPC_CMD
in entrypoint.sh.
---
Nitpick comments:
In `@ci/docker/entrypoint.sh`:
- Line 17: Add `set -u` alongside the existing `set -e` to enable
unbound-variable checks, and update any direct references to CUOPT_SERVER_TYPE
(the variable checked later) to avoid failing when it is intentionally unset —
for example, use parameter expansion like `${CUOPT_SERVER_TYPE:-}` in
comparisons or conditionals, or explicitly test for set/unset with `[ -z
"${CUOPT_SERVER_TYPE+x}" ]` before using it; update the code paths that
currently assume CUOPT_SERVER_TYPE exists (the conditional that handles server
type) to use one of these safe patterns.
🪄 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: 0498f788-ea53-4b3b-a767-69ca9740921e
📒 Files selected for processing (9)
.github/workflows/build_images.yamlGRPC_INTERFACE.mdGRPC_QUICK_START.mdGRPC_SERVER_ARCHITECTURE.mdci/docker/Dockerfileci/docker/entrypoint.shcpp/src/grpc/client/grpc_client.hppcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hpp
Comment on lines
+32
to
+35
| if [ -n "${CUOPT_GRPC_ARGS}" ]; then | ||
| read -ra EXTRA <<< "${CUOPT_GRPC_ARGS}" | ||
| GRPC_CMD+=("${EXTRA[@]}") | ||
| fi |
There was a problem hiding this comment.
Word-splitting caveat for CUOPT_GRPC_ARGS.
The read -ra approach splits on whitespace, so arguments containing embedded spaces (e.g., --arg "value with spaces") won't be preserved correctly. This is acceptable for typical CLI flags but worth noting in documentation if users might need complex arguments.
For most use cases (flags like --tls --log-to-console), the current implementation is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/docker/entrypoint.sh` around lines 32 - 35, The current block that reads
CUOPT_GRPC_ARGS into EXTRA via read -ra will split on whitespace and lose
embedded spaces in quoted arguments; either document this limitation or change
parsing to preserve quoted segments—replace the read -ra approach with a safe
eval-based split (e.g., eval "set -- $CUOPT_GRPC_ARGS" then append "$@" to
GRPC_CMD) so quoted values like "--arg \"value with spaces\"" are preserved;
update the comment above the block to mention CUOPT_GRPC_ARGS semantics and
reference CUOPT_GRPC_ARGS and GRPC_CMD in entrypoint.sh.
rgsl888prabhu
requested changes
Mar 31, 2026
Collaborator
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Awesome work @tmckayus, I have few comments, but rest looks good.
| # When CUOPT_SERVER_TYPE=grpc, the following env vars configure the gRPC server: | ||
| # CUOPT_SERVER_PORT — listen port (default: 5001) | ||
| # CUOPT_GPU_COUNT — worker processes (default: 1) | ||
| # CUOPT_GRPC_ARGS — additional CLI flags passed verbatim |
Collaborator
There was a problem hiding this comment.
Link to details on CUOPT_GRPC_ARGS or add more details.
Co-authored-by: Ramakrishnap <42624703+rgsl888prabhu@users.noreply.github.com>
tmckayus
force-pushed
the
grpc-docker-image
branch
from
0a2e0a7 to
f82bfc7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/cuopt/grpc/GRPC_QUICK_START.md (2)
167-167: Clarify default server type behavior.The table shows
CUOPT_SERVER_TYPEdefaults to*(unset)*but doesn't explain what happens in that case. Consider adding a note that when unset, the container defaults to the Python REST server.📝 Suggested clarification
-| `CUOPT_SERVER_TYPE` | *(unset)* | Set to `grpc` to launch the gRPC server instead of the REST server | +| `CUOPT_SERVER_TYPE` | *(unset)* | Set to `grpc` to launch the gRPC server. When unset, defaults to the Python REST server |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/grpc/GRPC_QUICK_START.md` at line 167, The table entry for the `CUOPT_SERVER_TYPE` environment variable is ambiguous about the unset/default behavior; update the documentation row (the line containing `CUOPT_SERVER_TYPE`) to explicitly state that when `CUOPT_SERVER_TYPE` is unset the container will default to the Python REST server (e.g., change the default column from "*(unset)*" to "*(unset) — defaults to Python REST server*" or add a short footnote below the table clarifying this default behavior).
151-161: Add prerequisite note for Docker GPU support.The Docker examples use
--gpus allbut don't explain this prerequisite. Consider adding a brief note that Docker GPU support requires nvidia-docker2 or Docker 19.03+ with nvidia-container-toolkit.📝 Suggested addition
Add before line 157:
+> **Note:** Running the gRPC server in Docker requires GPU access. Ensure Docker is configured with +> NVIDIA GPU support (nvidia-docker2 or Docker 19.03+ with nvidia-container-toolkit). + ```bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/grpc/GRPC_QUICK_START.md` around lines 151 - 161, Add a short prerequisite note above the Docker example in the "Docker" section explaining that the `--gpus all` flag requires Docker GPU support (e.g., nvidia-docker2 or Docker 19.03+ with the nvidia-container-toolkit) so users know to install/configure the NVIDIA container runtime before running the `docker run` command that sets `CUOPT_SERVER_TYPE=grpc`; update the text near the `--gpus all` example to include this requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/cuopt/grpc/GRPC_QUICK_START.md`:
- Line 167: The table entry for the `CUOPT_SERVER_TYPE` environment variable is
ambiguous about the unset/default behavior; update the documentation row (the
line containing `CUOPT_SERVER_TYPE`) to explicitly state that when
`CUOPT_SERVER_TYPE` is unset the container will default to the Python REST
server (e.g., change the default column from "*(unset)*" to "*(unset) — defaults
to Python REST server*" or add a short footnote below the table clarifying this
default behavior).
- Around line 151-161: Add a short prerequisite note above the Docker example in
the "Docker" section explaining that the `--gpus all` flag requires Docker GPU
support (e.g., nvidia-docker2 or Docker 19.03+ with the
nvidia-container-toolkit) so users know to install/configure the NVIDIA
container runtime before running the `docker run` command that sets
`CUOPT_SERVER_TYPE=grpc`; update the text near the `--gpus all` example to
include this requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43a5c107-84d3-42cf-a4a8-74aaf74d304b
📒 Files selected for processing (5)
CONTRIBUTING.mdGRPC_INTERFACE.mdci/docker/entrypoint.shdocs/cuopt/grpc/GRPC_QUICK_START.mddocs/cuopt/grpc/GRPC_SERVER_ARCHITECTURE.md
💤 Files with no reviewable changes (1)
- GRPC_INTERFACE.md
✅ Files skipped from review due to trivial changes (3)
- docs/cuopt/grpc/GRPC_SERVER_ARCHITECTURE.md
- ci/docker/entrypoint.sh
- CONTRIBUTING.md
rgsl888prabhu
requested changes
Apr 1, 2026
update docs for grpc remote execution feature
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/cuopt/source/_static/install-selector.js (1)
233-236: Clarify the CLI container variant restriction.
clinow advertisescontainer, but the UI still hides Release/CUDA andgetCommand()pins CLI to the stablecu12image. That means the selector cannot produce the other container tags already defined forCOMMANDS.c.container, and it never tells the user this is intentional. Either expose those choices for CLI containers or make the stable CUDA 12 restriction explicit in the output. As per coding guidelines, documentation changes should prioritize "Clarity: Flag confusing explanations, missing prerequisites, or unclear examples" and "Consistency: Version numbers, parameter types, and terminology match code".Also applies to: 342-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cuopt/source/_static/install-selector.js` around lines 233 - 236, The CLI entry in the install-type mapping currently claims "container" but the selector and getCommand() pin CLI to the stable cu12 image (and the UI hides Release/CUDA options), causing impossible container tag choices compared to COMMANDS.c.container; fix this by either removing "container" from the mapping for the "cli" key (so the array becomes ["pip","conda"]) or update getCommand() (the CLI branch) and the UI to expose the same container tag options used by COMMANDS.c.container (and ensure the selector lets users choose Release/CUDA), and if you keep the pin, make the restriction explicit in the CLI output text returned by getCommand() so users see that CLI is intentionally limited to the cu12/stable container image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 267-304: The selector's "container" branch currently only shows
the default server docker run (see variables hubPull and runLine and the cmd
construction) but omits the new gRPC service startup path; update the cmd string
in the Docker Hub and NGC branches (the blocks building cmd when registry !==
"ngc" and when registry === "ngc") to include either a commented alternative
docker run invoking the cuopt_grpc_server image with environment variables
CUOPT_SERVER_PORT (default 5001), CUOPT_GPU_COUNT, and CUOPT_GRPC_ARGS, or a
short commented pointer URL to the dedicated gRPC container docs; ensure the
message mentions the default port 5001 and uses the same tag/registry selection
(tag, hubPull) logic so users can discover the gRPC startup command from the
selector output.
---
Nitpick comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 233-236: The CLI entry in the install-type mapping currently
claims "container" but the selector and getCommand() pin CLI to the stable cu12
image (and the UI hides Release/CUDA options), causing impossible container tag
choices compared to COMMANDS.c.container; fix this by either removing
"container" from the mapping for the "cli" key (so the array becomes
["pip","conda"]) or update getCommand() (the CLI branch) and the UI to expose
the same container tag options used by COMMANDS.c.container (and ensure the
selector lets users choose Release/CUDA), and if you keep the pin, make the
restriction explicit in the CLI output text returned by getCommand() so users
see that CLI is intentionally limited to the cu12/stable container image.
🪄 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: 7a3ec9cb-21b4-4915-9a34-456a8ec0bf54
📒 Files selected for processing (19)
CONTRIBUTING.mdci/docker/entrypoint.shcpp/docs/DEVELOPER_GUIDE.mdcpp/docs/grpc-server-architecture.mddocs/cuopt/source/_static/install-selector.jsdocs/cuopt/source/cuopt-grpc/advanced.rstdocs/cuopt/source/cuopt-grpc/api.rstdocs/cuopt/source/cuopt-grpc/examples.rstdocs/cuopt/source/cuopt-grpc/examples/remote_lp_demo.mpsdocs/cuopt/source/cuopt-grpc/examples/remote_lp_demo.pydocs/cuopt/source/cuopt-grpc/grpc-server-architecture.mddocs/cuopt/source/cuopt-grpc/index.rstdocs/cuopt/source/cuopt-grpc/quick-start.rstdocs/cuopt/source/cuopt-server/index.rstdocs/cuopt/source/faq.rstdocs/cuopt/source/index.rstdocs/cuopt/source/install.rstdocs/cuopt/source/introduction.rstpython/cuopt/cuopt/linear_programming/data_model/data_model.py
✅ Files skipped from review due to trivial changes (16)
- docs/cuopt/source/install.rst
- docs/cuopt/source/introduction.rst
- cpp/docs/DEVELOPER_GUIDE.md
- python/cuopt/cuopt/linear_programming/data_model/data_model.py
- CONTRIBUTING.md
- docs/cuopt/source/faq.rst
- docs/cuopt/source/cuopt-server/index.rst
- docs/cuopt/source/cuopt-grpc/examples/remote_lp_demo.py
- docs/cuopt/source/cuopt-grpc/examples.rst
- docs/cuopt/source/cuopt-grpc/index.rst
- docs/cuopt/source/cuopt-grpc/examples/remote_lp_demo.mps
- docs/cuopt/source/cuopt-grpc/quick-start.rst
- docs/cuopt/source/cuopt-grpc/api.rst
- docs/cuopt/source/index.rst
- docs/cuopt/source/cuopt-grpc/advanced.rst
- docs/cuopt/source/cuopt-grpc/grpc-server-architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/docker/entrypoint.sh
Comment on lines
267
to
+304
| if (method === "container") { | ||
| var cudaKey = cuda || "cu12"; | ||
| var c = data[release][cudaKey] || data[release].cu12; | ||
| cmd = c.default + "\n\n# Run the container:\n" + c.run; | ||
| var hubPull = c.default; | ||
| var tag = "latest-cuda12.9-py3.13"; | ||
| var tm = hubPull.match(/docker pull nvidia\/cuopt:(\S+)/); | ||
| if (tm) tag = tm[1]; | ||
| var registry = getSelectedValue("cuopt-registry") || "hub"; | ||
| var runLine = c.run; | ||
| if (registry === "ngc" && release === "nightly") { | ||
| cmd = | ||
| "# Nightly cuOpt container images are not published to NVIDIA NGC; use Docker Hub for nightly builds.\n" + | ||
| "# (Select \"Docker Hub\" above for the same commands without this note.)\n\n" + | ||
| "# Docker Hub (docker.io) — no registry login required for public pulls\n" + | ||
| hubPull + | ||
| "\n\n" + | ||
| "# Run the container:\n" + | ||
| runLine; | ||
| } else if (registry === "ngc") { | ||
| runLine = runLine.replace(/nvidia\/cuopt:/g, "nvcr.io/nvidia/cuopt/cuopt:"); | ||
| cmd = | ||
| "# NVIDIA NGC (nvcr.io) — authenticate once per session, then pull:\n" + | ||
| "docker login nvcr.io\n" + | ||
| "# Username: $oauthtoken\n" + | ||
| "# Password: <NGC API key>\n\n" + | ||
| "docker pull nvcr.io/nvidia/cuopt/cuopt:" + | ||
| tag + | ||
| "\n\n" + | ||
| "# Run the container:\n" + | ||
| runLine; | ||
| } else { | ||
| cmd = | ||
| "# Docker Hub (docker.io) — no registry login required for public pulls\n" + | ||
| hubPull + | ||
| "\n\n" + | ||
| "# Run the container:\n" + | ||
| runLine; | ||
| } |
There was a problem hiding this comment.
Surface the gRPC container startup path in the selector.
For server + container, this branch still renders only the default server docker run. The PR adds a second public startup path via cuopt_grpc_server plus CUOPT_SERVER_PORT, CUOPT_GPU_COUNT, and CUOPT_GRPC_ARGS, so users entering through the selector cannot discover how to launch the gRPC server or that its default port is 5001. Please add either a commented alternative command here or a pointer to the dedicated gRPC container docs. As per coding guidelines, "Missing docs: If PR changes public APIs without updating docs, flag as HIGH priority."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cuopt/source/_static/install-selector.js` around lines 267 - 304, The
selector's "container" branch currently only shows the default server docker run
(see variables hubPull and runLine and the cmd construction) but omits the new
gRPC service startup path; update the cmd string in the Docker Hub and NGC
branches (the blocks building cmd when registry !== "ngc" and when registry ===
"ngc") to include either a commented alternative docker run invoking the
cuopt_grpc_server image with environment variables CUOPT_SERVER_PORT (default
5001), CUOPT_GPU_COUNT, and CUOPT_GRPC_ARGS, or a short commented pointer URL to
the dedicated gRPC container docs; ensure the message mentions the default port
5001 and uses the same tag/registry selection (tag, hubPull) logic so users can
discover the gRPC startup command from the selector output.
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.
The new cuopt_grpc_server can be run as an option from the cuopt docker image.
The default behavior of the image is the same, running the Python server, and the command for the container can be overridden from the docker command line.
The cuopt_grpc_server can be run explicitly from the docker command line
docker run![]()
cuopt_grpc_server [args...]
Or as an option, the cuopt_grpc_server can be selected with env vars in the docker environment:
CUOPT_SERVER_PORT — listen port (default: 5001)
CUOPT_GPU_COUNT — worker processes (default: 1)
CUOPT_GRPC_ARGS — additional CLI flags passed verbatim
This change also sets the default grpc port as 5001