Don't use SCCACHE_MEMCACHED_ENDPOINT if it is not valid#1406
Don't use SCCACHE_MEMCACHED_ENDPOINT if it is not valid#1406wendell-hom wants to merge 6 commits intomainfrom
Conversation
wendell-hom
requested review from
agirault and
wyli
and removed request for
Copilot
Greptile SummaryThis PR adds runtime validation of the Key changes:
The overall approach is sound. One minor observation: Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_environment_args called] --> B[Build sccache_keys from os.environ]
B --> C{enable_sccache?}
C -- No --> D[Warn if sccache keys present]
C -- Yes --> E[Forward HOLOHUB_ENABLE_SCCACHE + SCCACHE_DIR override]
E --> F[Loop over sccache_keys]
F --> G{k == SCCACHE_DIR?}
G -- Yes --> H[Skip always]
G -- No --> I{k == SCCACHE_MEMCACHED_ENDPOINT?}
I -- No --> J[Forward key unconditionally]
I -- Yes --> K[Call is_valid_endpoint]
K --> L[Parse host:port via rsplit]
L --> M{Parse OK?}
M -- No --> N[except Exception: warn + return False]
M -- Yes --> O[socket.create_connection timeout=5s]
O --> P{Connection OK?}
P -- Yes --> Q[info log + return True]
P -- No --> N
Q --> J
N --> H
Last reviewed commit: 996e865 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
|
WalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ 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. 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
🤖 Fix all issues with AI agents
In `@utilities/cli/container.py`:
- Around line 729-748: The is_valid_endpoint method references
socket.create_connection but the socket module is not imported; add "import
socket" to the top-level stdlib imports to fix the F821 lint error, and while
here, tighten the broad except in is_valid_endpoint (referencing
is_valid_endpoint, endpoint, host, port_str) to except (ValueError, OSError) so
only malformed port and connection errors are caught instead of swallowing all
exceptions.
99b53dc to
87595f5
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds validation for the SCCACHE_MEMCACHED_ENDPOINT environment variable before passing it to containers. The change prevents build failures when the memcached endpoint is configured but not reachable by checking endpoint connectivity before forwarding the environment variable.
Changes:
- Added
is_valid_endpoint()method to validate memcached endpoint reachability via socket connection - Modified environment variable forwarding logic to conditionally include
SCCACHE_MEMCACHED_ENDPOINTbased on validation
wendell-hom
force-pushed
the
whom/check-memcached-endpoint
branch
from
87595f5 to
0874a50
Compare
wendell-hom
force-pushed
the
whom/check-memcached-endpoint
branch
from
0a97e30 to
e873ba3
Compare
| # Forward other SCCACHE_* environment variables present on host | ||
| for k in sccache_keys: | ||
| if k != "SCCACHE_DIR": | ||
| if (k != "SCCACHE_DIR") and (k != "SCCACHE_MEMCACHED_ENDPOINT" or self.is_valid_endpoint()): |
There was a problem hiding this comment.
looks good to me, minor suggestions:
- add the env var to the printing util
Line 1308 in 2ac7c84
- add usage to readme:
holohub/utilities/cli/README.md
Line 445 in 2ac7c84
bhashemian
added
the
Action Required by Author
|
@wendell-hom what do you think about Wenqi's comment? Could you please respond? Thanks |
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 `@utilities/cli/container.py`:
- Line 782: The long conditional is exceeding Black's 88-char limit; split the
expression across lines and/or parenthesize it to wrap before 88 chars (e.g.,
break after the first comparison and place the second comparison on the next
line), keeping the same logic that checks k != "SCCACHE_DIR" and (k !=
"SCCACHE_MEMCACHED_ENDPOINT" or self.is_valid_endpoint()); update the line in
utilities/cli/container.py where that condition appears so it is
Black-compliant, then run the formatter (black or ./holohub lint --fix) to
ensure styling passes.
---
Duplicate comments:
In `@utilities/cli/container.py`:
- Around line 730-749: The is_valid_endpoint method uses a broad except
Exception; narrow it to catch the realistic failure modes (catch ValueError for
bad split/port parsing and OSError for connection failures) and handle them
similarly to the current behavior; update the except block(s) in
is_valid_endpoint to catch ValueError and OSError (using an exception variable
if you want to include details in the warn message) and leave other exceptions
to propagate.
| host, port_str = endpoint.rsplit(":", 1) | ||
| port = int(port_str) | ||
|
|
||
| with socket.create_connection((host, port), timeout=5): |
There was a problem hiding this comment.
Hardcoded 5-second blocking timeout
The 5-second timeout is hardcoded and applied synchronously, meaning every container launch with an unreachable SCCACHE_MEMCACHED_ENDPOINT will block for at least 5 seconds before showing the warning. Consider making the timeout configurable or reducing it (e.g., 1–2 seconds), since a user who misconfigured the endpoint will experience a noticeable pause on every invocation.
| with socket.create_connection((host, port), timeout=5): | |
| with socket.create_connection((host, port), timeout=2): |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utilities/cli/container.py (1)
782-783: Lift the memcached validation out of this predicate.Line 782 hides
self.is_valid_endpoint()inside what looks like simple env-var filtering. Precomputing it once before the loop keeps the control flow explicit and avoids burying I/O in a boolean expression.♻️ Refactor sketch
if enable_sccache: # Forward HOLOHUB_ENABLE_SCCACHE to enable launcher before cmake build args.extend(["-e", "HOLOHUB_ENABLE_SCCACHE"]) # Always set SCCACHE_DIR inside container to mounted path args.extend(["-e", f"SCCACHE_DIR={SCCACHE_CONTAINER_DIR}"]) + memcached_endpoint_valid = ( + self.is_valid_endpoint() + if "SCCACHE_MEMCACHED_ENDPOINT" in sccache_keys + else False + ) # Forward other SCCACHE_* environment variables present on host for k in sccache_keys: - if (k != "SCCACHE_DIR") and (k != "SCCACHE_MEMCACHED_ENDPOINT" or self.is_valid_endpoint()): + if k == "SCCACHE_DIR": + continue + if k == "SCCACHE_MEMCACHED_ENDPOINT" and not memcached_endpoint_valid: + continue args.extend(["-e", k])As per coding guidelines, "All code must adhere to Holoscan SDK coding standards for style compliance, descriptive naming, minimal abbreviations, inline documentation, and error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/cli/container.py` around lines 782 - 783, The current env-var filtering hides the call to self.is_valid_endpoint() inside the predicate and performs I/O during each iteration; refactor by calling self.is_valid_endpoint() once before the loop (e.g., memcached_valid = self.is_valid_endpoint()) and then replace the compound condition with a simple check using memcached_valid so the loop reads: if k != "SCCACHE_DIR" and (k != "SCCACHE_MEMCACHED_ENDPOINT" or memcached_valid): args.extend(["-e", k]); this removes repeated I/O from the boolean expression and makes control flow explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utilities/cli/container.py`:
- Around line 724-743: The is_valid_endpoint function performs a real TCP probe
(socket.create_connection) which must be skipped in dry-run; change
is_valid_endpoint to accept a dry_run flag (e.g., def is_valid_endpoint(self,
dry_run: bool = False)) and if dry_run is True, avoid creating a socket and
instead validate only the endpoint format (split on ":" and int(port)) and log
or info that probing was skipped; update all call sites that construct the
Docker/CLI command to pass the current dry-run state (e.g., self.args.dry_run or
parser.dry_run) into is_valid_endpoint so dry-run printing never does network
I/O.
---
Nitpick comments:
In `@utilities/cli/container.py`:
- Around line 782-783: The current env-var filtering hides the call to
self.is_valid_endpoint() inside the predicate and performs I/O during each
iteration; refactor by calling self.is_valid_endpoint() once before the loop
(e.g., memcached_valid = self.is_valid_endpoint()) and then replace the compound
condition with a simple check using memcached_valid so the loop reads: if k !=
"SCCACHE_DIR" and (k != "SCCACHE_MEMCACHED_ENDPOINT" or memcached_valid):
args.extend(["-e", k]); this removes repeated I/O from the boolean expression
and makes control flow explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70b3f248-89f4-40a2-b5e4-c1f4d3599a4e
📒 Files selected for processing (1)
utilities/cli/container.py
| def is_valid_endpoint(self) -> bool: | ||
| """Check if SCCACHE_MEMCACHED_ENDPOINT is valid""" | ||
| endpoint = os.environ.get("SCCACHE_MEMCACHED_ENDPOINT") | ||
|
|
||
| if endpoint: | ||
| try: | ||
| host, port_str = endpoint.rsplit(":", 1) | ||
| port = int(port_str) | ||
|
|
||
| with socket.create_connection((host, port), timeout=5): | ||
| info(f" > Using memcached endpoint {endpoint}") | ||
| return True | ||
|
|
||
| except Exception: | ||
| warn( | ||
| f" > Memcached endpoint {endpoint} is not reachable, " | ||
| "falling back to local caching." | ||
| ) | ||
|
|
||
| return False |
There was a problem hiding this comment.
Don't probe the memcached server in dry-run mode.
Line 733 opens a real TCP connection during argument construction. That makes dry-run mode depend on host reachability and can add a 5-second stall just to print the Docker command.
💡 Suggested direction
def is_valid_endpoint(self) -> bool:
"""Check if SCCACHE_MEMCACHED_ENDPOINT is valid"""
endpoint = os.environ.get("SCCACHE_MEMCACHED_ENDPOINT")
+ if self.dryrun:
+ return bool(endpoint)
if endpoint:
try:
host, port_str = endpoint.rsplit(":", 1)
port = int(port_str)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_valid_endpoint(self) -> bool: | |
| """Check if SCCACHE_MEMCACHED_ENDPOINT is valid""" | |
| endpoint = os.environ.get("SCCACHE_MEMCACHED_ENDPOINT") | |
| if endpoint: | |
| try: | |
| host, port_str = endpoint.rsplit(":", 1) | |
| port = int(port_str) | |
| with socket.create_connection((host, port), timeout=5): | |
| info(f" > Using memcached endpoint {endpoint}") | |
| return True | |
| except Exception: | |
| warn( | |
| f" > Memcached endpoint {endpoint} is not reachable, " | |
| "falling back to local caching." | |
| ) | |
| return False | |
| def is_valid_endpoint(self) -> bool: | |
| """Check if SCCACHE_MEMCACHED_ENDPOINT is valid""" | |
| endpoint = os.environ.get("SCCACHE_MEMCACHED_ENDPOINT") | |
| if self.dryrun: | |
| return bool(endpoint) | |
| if endpoint: | |
| try: | |
| host, port_str = endpoint.rsplit(":", 1) | |
| port = int(port_str) | |
| with socket.create_connection((host, port), timeout=5): | |
| info(f" > Using memcached endpoint {endpoint}") | |
| return True | |
| except Exception: | |
| warn( | |
| f" > Memcached endpoint {endpoint} is not reachable, " | |
| "falling back to local caching." | |
| ) | |
| return False |
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 737-737: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utilities/cli/container.py` around lines 724 - 743, The is_valid_endpoint
function performs a real TCP probe (socket.create_connection) which must be
skipped in dry-run; change is_valid_endpoint to accept a dry_run flag (e.g., def
is_valid_endpoint(self, dry_run: bool = False)) and if dry_run is True, avoid
creating a socket and instead validate only the endpoint format (split on ":"
and int(port)) and log or info that probing was skipped; update all call sites
that construct the Docker/CLI command to pass the current dry-run state (e.g.,
self.args.dry_run or parser.dry_run) into is_valid_endpoint so dry-run printing
never does network I/O.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utilities/cli/container.py (1)
724-743:⚠️ Potential issue | 🟠 MajorKeep dry-run side-effect free.
run()still reaches this helper while building the command, so--dry-runopens a real TCP connection here and can stall for 5 seconds just to print Docker args. Please thread the dry-run state intois_valid_endpoint()and skip the network probe in dry-run mode, while still validating thehost:portformat locally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/cli/container.py` around lines 724 - 743, The is_valid_endpoint function currently performs a real network probe which causes side effects during --dry-run; change its signature (or add an optional parameter) to accept a dry_run boolean from run() and when dry_run is True only validate the SCCACHE_MEMCACHED_ENDPOINT format (split on ":" and ensure port is an int and host is non-empty) but do not call socket.create_connection; when dry_run is False keep the existing reachability probe and logging behavior. Update callers (e.g., run()) to pass the dry-run state into is_valid_endpoint so building the command remains side-effect free in dry-run mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@utilities/cli/container.py`:
- Around line 724-743: The is_valid_endpoint function currently performs a real
network probe which causes side effects during --dry-run; change its signature
(or add an optional parameter) to accept a dry_run boolean from run() and when
dry_run is True only validate the SCCACHE_MEMCACHED_ENDPOINT format (split on
":" and ensure port is an int and host is non-empty) but do not call
socket.create_connection; when dry_run is False keep the existing reachability
probe and logging behavior. Update callers (e.g., run()) to pass the dry-run
state into is_valid_endpoint so building the command remains side-effect free in
dry-run mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dfb4c7f-971c-49c8-81f2-7646f6dada88
📒 Files selected for processing (1)
utilities/cli/container.py
|
@wendell-hom Could you please address the comments on this PR? Thanks. |
Don't pass the sccache memcached endpoint env variable if the server is not reachable, otherwise compilation will fail.
Summary by CodeRabbit