add a scriptlet to put cuopt_grpc_server on the default path#1100
Open
tmckayus wants to merge 5 commits intoNVIDIA:mainfrom
Open
add a scriptlet to put cuopt_grpc_server on the default path#1100tmckayus wants to merge 5 commits intoNVIDIA:mainfrom
tmckayus wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
|
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 new CLI entry point Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
rgsl888prabhu
added
non-breaking
rgsl888prabhu
approved these changes
Apr 14, 2026
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.py`:
- Around line 14-26: The test_cuopt_grpc_server_help currently calls
subprocess.run on the bare "cuopt_grpc_server" and only checks stdout, which can
raise FileNotFoundError and miss help output printed to stderr; update the test
to first locate the executable with shutil.which("cuopt_grpc_server") and call
pytest.skip if not found, or alternatively invoke the module via sys.executable
-m if an importable entrypoint exists, then call subprocess.run with
stderr=subprocess.STDOUT (or combine stdout/stderr) and a controlled env (e.g.,
env=os.environ.copy() with GPU vars like CUDA_VISIBLE_DEVICES cleared) so the
call is stream-agnostic and environment-isolated, and assert on the combined
output and returncode.
🪄 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: 9c80b148-7cb0-405c-b790-842b0fdb0941
📒 Files selected for processing (4)
conda/recipes/cuopt-server/recipe.yamlpython/cuopt_server/cuopt_server/_grpc_server_wrapper.pypython/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.pypython/cuopt_server/pyproject.toml
✅ Files skipped from review due to trivial changes (1)
- python/cuopt_server/pyproject.toml
rgsl888prabhu
approved these changes
Apr 14, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/libcuopt/libcuopt/_grpc_server_wrapper.py`:
- Around line 13-16: The code currently constructs server_path and calls
subprocess.call([server_path] + sys.argv[1:]) which will raise a traceback if
the binary is missing or not executable; update the wrapper to first check
server_path exists and is executable (use os.path.exists/server_path and
os.access(server_path, os.X_OK)), and if not, write a clear error message to
stderr and exit with a non‑zero code; additionally wrap the subprocess.call
invocation in a try/except to catch OSError/CalledProcessError and print the
captured error details before calling sys.exit(1) so failures are reported
cleanly.
🪄 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: 9ddac6bf-6c79-4a56-a8fd-ca3fae24b360
📒 Files selected for processing (3)
python/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.pypython/libcuopt/libcuopt/_grpc_server_wrapper.pypython/libcuopt/pyproject.toml
✅ Files skipped from review due to trivial changes (1)
- python/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.py
Comment on lines
+13
to
+16
| server_path = os.path.join( | ||
| os.path.dirname(__file__), "bin", "cuopt_grpc_server" | ||
| ) | ||
| sys.exit(subprocess.call([server_path] + sys.argv[1:])) |
There was a problem hiding this comment.
Handle missing/non-executable binary gracefully.
On Line 16, a missing or non-executable binary will surface as a Python traceback. Add an explicit guard and exit with a clear error message.
Suggested hardening patch
def main():
@@
server_path = os.path.join(
os.path.dirname(__file__), "bin", "cuopt_grpc_server"
)
- sys.exit(subprocess.call([server_path] + sys.argv[1:]))
+ if not os.path.isfile(server_path) or not os.access(server_path, os.X_OK):
+ print(
+ f"cuopt_grpc_server binary not found or not executable: {server_path}",
+ file=sys.stderr,
+ )
+ sys.exit(127)
+ sys.exit(subprocess.call([server_path] + sys.argv[1:]))🧰 Tools
🪛 Ruff (0.15.10)
[error] 16-16: subprocess call: check for execution of untrusted input
(S603)
[warning] 16-16: Consider [server_path, *sys.argv[1:]] instead of concatenation
Replace with [server_path, *sys.argv[1:]]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/libcuopt/libcuopt/_grpc_server_wrapper.py` around lines 13 - 16, The
code currently constructs server_path and calls subprocess.call([server_path] +
sys.argv[1:]) which will raise a traceback if the binary is missing or not
executable; update the wrapper to first check server_path exists and is
executable (use os.path.exists/server_path and os.access(server_path, os.X_OK)),
and if not, write a clear error message to stderr and exit with a non‑zero code;
additionally wrap the subprocess.call invocation in a try/except to catch
OSError/CalledProcessError and print the captured error details before calling
sys.exit(1) so failures are reported cleanly.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/libcuopt/libcuopt/_grpc_server_wrapper.py (1)
13-16:⚠️ Potential issue | 🟡 MinorHandle missing binary and spawn failures with explicit user-facing errors.
If the packaged binary is missing/non-executable (or
execvefails), this currently produces a Python traceback instead of a clear CLI error. Please add a guard +OSErrorhandling before exiting.Suggested patch
def main(): @@ server_path = os.path.join( os.path.dirname(__file__), "bin", "cuopt_grpc_server" ) - sys.exit(subprocess.call([server_path] + sys.argv[1:])) + if not os.path.isfile(server_path) or not os.access(server_path, os.X_OK): + print( + f"cuopt_grpc_server binary not found or not executable: {server_path}", + file=sys.stderr, + ) + sys.exit(127) + try: + sys.exit(subprocess.call([server_path, *sys.argv[1:]])) + except OSError as exc: + print(f"failed to execute cuopt_grpc_server: {exc}", file=sys.stderr) + sys.exit(1)#!/bin/bash set -euo pipefail # Verify whether grpc wrapper has existence/X_OK checks and spawn error handling. nl -ba python/libcuopt/libcuopt/_grpc_server_wrapper.py | sed -n '1,120p' echo rg -n 'os\.path\.(exists|isfile)|os\.access|try:|except OSError|subprocess\.(call|run)' \ python/libcuopt/libcuopt/_grpc_server_wrapper.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/libcuopt/libcuopt/_grpc_server_wrapper.py` around lines 13 - 16, The wrapper should check that server_path (constructed in this file) exists and is executable before attempting to spawn it, and wrap the subprocess invocation (currently using subprocess.call and sys.exit) in a try/except that catches OSError (and optionally subprocess.SubprocessError) to emit a clear user-facing error (to stderr or via logging) and exit with a non-zero status; update the code around server_path, subprocess.call(...) and sys.exit(...) to perform os.path.exists/os.access checks and handle spawn failures with a descriptive OSError message including server_path and the underlying exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/libcuopt/libcuopt/_grpc_server_wrapper.py`:
- Around line 13-16: The wrapper should check that server_path (constructed in
this file) exists and is executable before attempting to spawn it, and wrap the
subprocess invocation (currently using subprocess.call and sys.exit) in a
try/except that catches OSError (and optionally subprocess.SubprocessError) to
emit a clear user-facing error (to stderr or via logging) and exit with a
non-zero status; update the code around server_path, subprocess.call(...) and
sys.exit(...) to perform os.path.exists/os.access checks and handle spawn
failures with a descriptive OSError message including server_path and the
underlying exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 64463483-a778-4bbf-a846-f40b409d4840
📒 Files selected for processing (3)
python/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.pypython/libcuopt/libcuopt/_grpc_server_wrapper.pypython/libcuopt/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- python/libcuopt/pyproject.toml
- python/cuopt_server/cuopt_server/tests/test_grpc_server_entry_point.py
this is necessary for cuopt_grpc_server binary to be visible in CI on amd64
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.
This changes adds a scriptlet for cuopt_grpc_server so that after installation of the cuopt-server-cuXX packages the binary is on the default path, similar to cuopt_cli.