Skip to content

Address #177 review comments: harden image-template formatting + base-image docs#178

Open
wyli wants to merge 1 commit into
mainfrom
wenqil/pr177-review-followups
Open

Address #177 review comments: harden image-template formatting + base-image docs#178
wyli wants to merge 1 commit into
mainfrom
wenqil/pr177-review-followups

Conversation

@wyli

@wyli wyli commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #177 (merged), addressing its review comments.

Changes

container/core.py

  • _format_image_template: parse the HOLOSCAN_CLI_*_IMAGE_FORMAT placeholders with string.Formatter and fatal() on unknown/missing fields or an invalid template, instead of letting a mistyped or unset placeholder raise a raw KeyError/ValueError traceback. Parsing the fields (rather than a substring check) also fixes "{sdk_version" in template misfiring on escaped {{sdk_version}}. — coderabbitai, Copilot
  • default_base_image: compute cuda_tag lazily so the host GPU/driver probe (and its warning) no longer runs on the branches that return an explicit image or fatal() without using it. — Copilot

README.md

  • Rewrite the base-image configuration section into three explicit methods (--base-img flag, fully-qualified HOLOSCAN_CLI_BASE_IMAGE, or repository + HOLOSCAN_CLI_BASE_SDK_VERSION). — agirault

tests

  • New test_container_core.py cases for the _format_image_template validation paths and the no-host-probe fatal path in default_base_image.
  • Clarifying comment in test_get_cuda_tag_handles_sdk_and_cuda_matrix explaining the stubbed host probes (why a None cuda_version resolves to an igpu tag). — tbirdso

Testing

  • black --check + isort --check clean; pytest tests/unit green (the wheel-install console-script test requires an installed dist, unrelated to this change).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Clarified that holoscan-cli version alignment doesn't require matching Holoscan SDK runtime or container image
    • Expanded base image configuration guidance with multiple options: --base-img flag, HOLOSCAN_CLI_BASE_IMAGE environment variable, and dynamic CUDA version detection
    • Updated behavior to prompt for base image when unconfigured
  • Bug Fixes

    • Enhanced validation for base image template configurations with explicit error messaging

container/core.py:
- _format_image_template: validate HOLOSCAN_CLI_*_IMAGE_FORMAT placeholders
  with string.Formatter and fail via fatal() on unknown/missing fields or an
  invalid template, instead of a raw KeyError/ValueError traceback. Parsing
  the fields (vs. a substring check) also stops escaped braces like
  {{sdk_version}} from being mistaken for a real sdk_version placeholder.
  (coderabbitai, Copilot)
- default_base_image: compute cuda_tag lazily so the host GPU/driver probe
  (and its warning) no longer runs on the branches that return an explicit
  image or fatal without using it. (Copilot)

README.md:
- Rewrite the base-image configuration section as three explicit methods
  (--base-img flag, fully-qualified HOLOSCAN_CLI_BASE_IMAGE, or repository +
  HOLOSCAN_CLI_BASE_SDK_VERSION). (agirault)

tests:
- Cover the new _format_image_template validation paths and the no-host-probe
  fatal path in default_base_image.
- Document the stubbed host probes in test_get_cuda_tag_handles_sdk_and_cuda_matrix
  (clarifies why a None cuda_version resolves to an igpu tag). (tbirdso)

Follow-up to #177.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@wyli wyli requested review from agirault and tbirdso June 2, 2026 16:22
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7eff6344-de2d-4f3c-a101-75236a4e3d70

📥 Commits

Reviewing files that changed from the base of the PR and between c0ce0e7 and 97378f4.

📒 Files selected for processing (4)
  • README.md
  • src/holoscan_cli/container/core.py
  • tests/unit/test_container_core.py
  • tests/unit/test_sdk_utils.py

Walkthrough

The PR refactors base image template handling in the Holoscan CLI container module. It improves placeholder validation in image-template formatting, defers CUDA tag computation to only the paths requiring it, and updates documentation to clarify base image configuration options and fallback behavior.

Changes

Base image template and configuration

Layer / File(s) Summary
Template formatting refactor with placeholder validation
src/holoscan_cli/container/core.py, tests/unit/test_container_core.py
string.Formatter().parse() replaces substring-based placeholder checking, explicitly validating that referenced template fields exist and required values are provided; fatal errors raised for unknown fields, missing values, and invalid format operations.
Lazy CUDA tag evaluation in default_base_image
src/holoscan_cli/container/core.py, tests/unit/test_container_core.py
cuda_tag computation via get_cuda_tag() shifts from eager evaluation at method entry to lazy computation only in the BASE_SDK_VERSION fallback path, avoiding unnecessary host probing when explicit base image is configured.
Documentation updates for base image configuration
README.md, tests/unit/test_sdk_utils.py
README versioning section restructured to explain three base image configuration methods (flag, full image env var, env vars with dynamic CUDA tag derivation) and updated fallback prompt behavior; test comments clarify deterministic host stubs for CUDA resolution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • nvidia-holoscan/holoscan-cli#177: Modifies HoloscanContainer base-image and defaulting logic in the same file with overlapping changes to default_base_image behavior and corresponding unit test coverage.

Suggested reviewers

  • tbirdso
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: addressing review comments from #177 with improvements to image-template formatting validation and base-image configuration documentation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant