fix: honor explicit Docker runtime before checking nvidia-ctk#199
Conversation
Co-authored-by: Codex <noreply@openai.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
WalkthroughHoloscanContainer.run() now parses configured docker run arguments early to detect an explicit --runtime option and conditionally skips NVIDIA Container Toolkit checks when the resolved runtime isn't "nvidia". get_gpu_runtime_args() gains an optional runtime parameter controlling injected runtime flags. Tests updated accordingly. ChangesCustom runtime handling
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: None specified. Related PRs: None specified. Suggested labels: bug, tests Suggested reviewers: None specified. 🐰 A runtime whispered, "runc, not nvidia today," 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/unit/test_container_core.py (1)
496-513: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep a default-runtime CTK assertion in this file.
tests/unit/test_container_core.pyno longer has a test that runsc.run()without--runtimeand assertscheck_nvidia_ctk()is called, so this branch is now uncovered.🤖 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 `@tests/unit/test_container_core.py` around lines 496 - 513, Add back a unit test in test_run_assembles_docker_command_without_ctk_for_custom_runtime or nearby that calls c.run() without specifying a custom --runtime and asserts check_nvidia_ctk() is invoked; the current test only covers the custom-runtime path. Use the existing container_core, run_command, and check_nvidia_ctk symbols to locate the right branch and keep the default-runtime NVIDIA Container Toolkit validation covered.
🧹 Nitpick comments (1)
tests/unit/test_container_core.py (1)
557-559: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting no duplicate
--runtimeflag.Add
assert cmd.count("--runtime") == 1alongside the existing--cidfileduplicate check, to directly guard againstget_gpu_runtime_argsre-injecting--runtimewhen a custom one is already present indocker_opts.✅ Suggested addition
assert container_core.get_cli_arg_value(cmd, "--runtime") == "runc" + assert cmd.count("--runtime") == 1 assert not ctk_checks🤖 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 `@tests/unit/test_container_core.py` around lines 557 - 559, The test around get_gpu_runtime_args should also verify that a custom --runtime passed through docker_opts is not duplicated in the final cmd. Update the existing assertions in the container core test to add a direct count check for --runtime, alongside the current cidfile duplicate guard, so the test explicitly catches any re-injection of the runtime flag while still validating the resolved runtime value from get_cli_arg_value.
🤖 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.
Outside diff comments:
In `@tests/unit/test_container_core.py`:
- Around line 496-513: Add back a unit test in
test_run_assembles_docker_command_without_ctk_for_custom_runtime or nearby that
calls c.run() without specifying a custom --runtime and asserts
check_nvidia_ctk() is invoked; the current test only covers the custom-runtime
path. Use the existing container_core, run_command, and check_nvidia_ctk symbols
to locate the right branch and keep the default-runtime NVIDIA Container Toolkit
validation covered.
---
Nitpick comments:
In `@tests/unit/test_container_core.py`:
- Around line 557-559: The test around get_gpu_runtime_args should also verify
that a custom --runtime passed through docker_opts is not duplicated in the
final cmd. Update the existing assertions in the container core test to add a
direct count check for --runtime, alongside the current cidfile duplicate guard,
so the test explicitly catches any re-injection of the runtime flag while still
validating the resolved runtime value from get_cli_arg_value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e802d82f-8b52-4dbe-9fe2-6ab6cdaf25f8
📒 Files selected for processing (2)
src/holoscan_cli/container/core.pytests/unit/test_container_core.py
2 participants
Summary
Honor an explicitly supplied Docker
--runtimefrom--docker-optsbefore validatingnvidia-ctk.Launches without a runtime override retain the existing NVIDIA runtime and NVIDIA Container Toolkit check.
Context
The container launcher currently performs the
nvidia-ctkpreflight before Docker receives caller-provided options. This blocks an explicit alternative runtime such as--docker-opts='--runtime runc'before Docker can use it.Scope
This is a narrow preflight fix for callers that explicitly select a non-NVIDIA Docker runtime. It does not add or claim official support for running Holoscan workloads without the NVIDIA Container Toolkit, GPU-free configurations, or alternative runtimes generally.
Existing NVIDIA runtime behavior and GPU-related Docker arguments are unchanged.
Validation
PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=src pytest -q -o addopts='' tests/unit/test_container_core.pyAI-assisted: Created with Codex/GPT at the user's request.
Summary by CodeRabbit