Skip to content

fix: TEM-31, TEM-32, TEM-39, TEM-37 pip warm-up, RunPod REST API v2 in smoke tests, proxy-first checks - #30

Merged
Chmokachka merged 11 commits into
mainfrom
fix/TEM-31-unavailable-pip-and-uv
Aug 14, 2026
Merged

fix: TEM-31, TEM-32, TEM-39, TEM-37 pip warm-up, RunPod REST API v2 in smoke tests, proxy-first checks#30
Chmokachka merged 11 commits into
mainfrom
fix/TEM-31-unavailable-pip-and-uv

Conversation

@Chmokachka

@Chmokachka Chmokachka commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Closes TEM-31, TEM-32, TEM-37, TEM-39.

Summary

Image fixes

  • pip warm-up on cold start (TEM-31). ComfyUI-Manager probes python -m pip --version with a 5s timeout; on a cold container the first pip invocation could exceed it (slow network volume) and Manager reported "Neither pip nor uv are available". start.sh now warms pip up before ComfyUI starts and logs the wall time. A matching always-on pip check in the smoke tests fails the run if pip is missing or slower than Manager's 5s budget. The check measures wall time in milliseconds via bash 5's $EPOCHREALTIME (whole-second timestamps would record a ~5.9s run as "5s" and let it slip under the budget), logging pip_wall_ms=…. Deliberately timed with shell built-ins — a python-based clock would pre-warm the interpreter from the network volume and bias the cold-start cost being measured.
  • Accurate shutdown vs. crash logging (TEM-37). Previously every pod stop/restart/terminate printed a misleading "ComfyUI crashed — check the logs above" banner, because the SIGTERM path was indistinguishable from a real crash. start.sh now tracks SIGTERM/SIGINT in the trap: on a normal shutdown it logs a single "Pod is shutting down" line, cleanly stops Jupyter and FileBrowser (Docker only signals PID 1), and exits. The crash banner is printed only when ComfyUI itself dies, and now includes the actual exit code (e.g. 137 for OOM-kill vs. 1/2 for a Python error). A real crash still keeps the container alive via sleep infinity, so SSH/Jupyter stay accessible — that behavior is unchanged.

Note: the earlier NVIDIA_DRIVER_CAPABILITIES trim was reverted (stays all) — it would disable the core GLSL node's EGL/OpenGL/Vulkan path in the pinned ComfyUI v0.30.0. The headless-driver host issue will be addressed separately.

Smoke tests: RunPod REST API v2 adoption (TEM-32)

  • Fail-fast pod status via GET /v2/pods/{id}. SSH stays the readiness gate, but the v2 status is polled alongside: unlike the legacy desiredStatus (RUNNING from second one, even when the container never starts), v2 distinguishes STARTING/RUNNING and surfaces ERROR — on ERROR/EXITED/TERMINATED we bail immediately instead of sitting out the full CREATE_TIMEOUT.
  • Container logs over the API instead of SSH. Container stdout is now fetched via the v2 logs endpoint (SSH couldn't see PID 1 stdout at all); SSH is kept only for the GPU SMI snapshot. Logs are scanned for error markers (LOG_ERROR_PATTERN, now also matching crash(ed/es/ing)), and the scan fails the run on a match. An empty fetch is never a pass: the image always logs on boot, so an empty or failed fetch is retried (3×, 10s apart) and then FAILs as log scan unverified instead of silently passing.
  • Host-side system logs (GET /v2/pods/{id}/logs?source=system) on stall/timeout/terminal state and in every diagnostic dump. That's where image-pull and container-init failures are reported — container stdout is empty when the container never started. Filtered by the new SYS_LOG_ERROR_PATTERN (error/failed/crashed markers).

Smoke tests: reliability & UX

  • Proxy-first checks with a strict success criterion. Jupyter / generic port / ComfyUI reachability probes now hit the public RunPod proxy first (the end-user path); the in-pod SSH probe runs only as a diagnostic when the proxy fails, to distinguish "service never started" from "port not exposed as http". The proxy probe requires a strict HTTP 200: anything else — notably the 404 the RunPod proxy emits on its own while the pod isn't in its routing table yet — is retried until PORT_PROXY_TIMEOUT and then fails, so a transient proxy 404 can't masquerade as a healthy service. Every app we test serves 200 on / (redirects are followed, so a healthy redirect chain still ends in 200); the in-pod diagnostic probe (localhost, no proxy in the middle) keeps the looser <500 criterion.
  • Post-dwell re-verification. The SSH re-probe after the dwell window couldn't catch a late ComfyUI death — start.sh keeps the container (and SSH) alive via sleep infinity after a crash, and the log scan used to run before dwell. After the dwell the runner now re-probes ComfyUI's /system_stats via the proxy (when the group tests ComfyUI) and re-runs the container-log error scan, so anything that crashed or logged errors during the window fails the pair.
  • De-flaked the RunpodDirect feature-detect. GET /server_download/folder_paths was a single-shot probe, so a transient proxy 404/5xx was misclassified as "node not installed in this image" (intermittent CI failures). It now retries for up to COMFYUI_ROUTES_TIMEOUT (default 60s) and the FAIL message includes the last HTTP code/error.
  • Readable parallel logs. Worker tags now include the GPU instance under test ([W2-A100 SXM 40GB] instead of [W2]), so interleaved lines from parallel jobs are attributable at a glance.

Pipeline examples:

@Chmokachka Chmokachka changed the title fix: TEM-31 fix: TEM-31, TEM-32, TEM-39 pip warm-up, RunPod REST API v2 in smoke tests, proxy-first checks Aug 7, 2026
@Chmokachka
Chmokachka marked this pull request as ready for review August 10, 2026 09:19
Co-authored-by: mchekm <mchekm@softserveinc.com>
@Chmokachka Chmokachka changed the title fix: TEM-31, TEM-32, TEM-39 pip warm-up, RunPod REST API v2 in smoke tests, proxy-first checks fix: TEM-31, TEM-32, TEM-39, TEM-37 pip warm-up, RunPod REST API v2 in smoke tests, proxy-first checks Aug 10, 2026
@kodxana

kodxana commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Thanks, I rechecked the current head (677202f) and its exact-head Actions runs. The available-GPU test results look good. Both CUDA 12.8 and CUDA 13 completed 9 tests with 0 failures. The remaining 31 cases in each matrix were skipped because capacity was unavailable or stuck.

I still see four issues that should be addressed before merging:

  1. Removing the NVIDIA graphics capability creates a compatibility regression.

    The image pins ComfyUI v0.30.0, which includes a [core GLSL node using EGL/OpenGL and ANGLE Vulkan](https://github.com/Comfy-Org/ComfyUI/blob/v0.30.0/comfy_extras/nodes_glsl.py). [NVIDIA documents](https://docs.nvidia.com/datacenter/cloud-native/container-toolkit/latest/docker-specialized.html) that the graphics capability is required for OpenGL and Vulkan.

    Please either retain support for graphics, or explicitly document that GLSL and Vulkan nodes are disabled by default, explain how users can enable them on compatible hosts, and add a targeted GLSL smoke test. The current generation test does not cover this path.

  2. The proxy health check accepts every HTTP status below 500, including 404.

    A transient proxy-generated 404 can therefore be treated as a healthy service. The runner then skips the in-pod verification. Please restrict success to expected responses, possibly allowing intentional 401 or 403 responses, and retry or fail on 404.

  3. The final log scan runs before the dwell period.

    If ComfyUI exits during dwell, start.sh keeps the container and SSH alive using sleep infinity. The SSH dwell probe can still pass, while the late ComfyUI failure is never scanned. Please perform another log scan after dwell and preferably probe ComfyUI again at that point.

  4. An empty log response is treated as a successful scan.

    scanned 0 log lines — no error markers should be considered unverified. Since this image always produces startup logs, an empty result should be retried or treated as a failure instead of passing.

Minor issue: the pip latency test uses whole-second timestamps and accepts <= 5. A command taking almost 6 seconds can still be recorded as 5 seconds. Please use a real five-second timeout or sub-second monotonic timing.

Once these issues are addressed and covered by targeted checks, I can review the updated version again.

@Madiator2011Work Madiator2011Work left a comment

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.

LGTM

@Chmokachka
Chmokachka merged commit 846fe0b into main Aug 14, 2026
7 of 8 checks passed
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.

4 participants