Skip to content

Don't use SCCACHE_MEMCACHED_ENDPOINT if it is not valid #1406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wendell-hom wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Don't use SCCACHE_MEMCACHED_ENDPOINT if it is not valid #1406

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
649d5f4
Don't use SCCACHE_MEMCACHED_ENDPOINT if it is not valid
wendell-hom Feb 7, 2026
e873ba3
Update to use info / warn
whom3 Feb 15, 2026
baf0282
Merge branch 'main' into whom/check-memcached-endpoint
bhashemian Feb 20, 2026
0c9f6bc
Merge branch 'main' into whom/check-memcached-endpoint
bhashemian Mar 7, 2026
3b542c4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 7, 2026
996e865
Merge branch 'main' into whom/check-memcached-endpoint
bhashemian Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion utilities/cli/container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import re
import shlex
import shutil
import socket
import stat
import subprocess
import sys
Expand Down Expand Up @@ -726,6 +727,27 @@ def get_gpu_runtime_args(self) -> List[str]:
args.extend(self.get_device_cgroup_args())
return args

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)
Comment thread
wendell-hom marked this conversation as resolved.
Comment thread
wendell-hom marked this conversation as resolved.

with socket.create_connection((host, port), timeout=5):
Comment thread
wendell-hom marked this conversation as resolved.

@greptile-apps greptile-apps Bot Mar 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
with socket.create_connection((host, port), timeout=5):
with socket.create_connection((host, port), timeout=2):

info(f" > Using memcached endpoint {endpoint}")
return True

except Exception:
warn(
f" > Memcached endpoint {endpoint} is not reachable, "
"falling back to local caching."
)
Comment thread
wendell-hom marked this conversation as resolved.

return False
Comment thread
wendell-hom marked this conversation as resolved.
Comment on lines +729 to +748

@coderabbitai coderabbitai Bot Mar 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


def get_environment_args(self) -> List[str]:
"""Environment variable arguments"""
args = [
Expand Down Expand Up @@ -757,7 +779,7 @@ def get_environment_args(self) -> List[str]:
args.extend(["-e", f"SCCACHE_DIR={SCCACHE_CONTAINER_DIR}"])
# 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()):

@wyli wyli Feb 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, minor suggestions:

  • add the env var to the printing util

    holohub/utilities/cli/util.py

    Line 1308 in 2ac7c84

    "HOLOHUB_ENABLE_SCCACHE",
  • add usage to readme:

    holohub/utilities/cli/README.md

    Line 445 in 2ac7c84

    - **`HOLOHUB_ENABLE_SCCACHE`**: Defaults to `false`. Set to `true` to enable rapids-sccache for the build. You can configure sccache with `SCCACHE_*` environment variables per the [sccache documentation](https://github.com/rapidsai/sccache/tree/rapids/docs). Use `--extra-scripts sccache` to install sccache in the container image (e.g., `./holohub build-container --extra-scripts sccache`).

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
args.extend(["-e", k])
elif len(sccache_keys) > 0:
warn(
Expand Down
Loading