Skip to content

Commit 2a35cb5

Browse files
AndreiOrehovclaude
andauthored
Fix the docker gate: absolute paths are platform-dependent, and the smoke test counted nine nodes (#10)
`docker-tests` has been red on main since at least 06:46 today, and on every push before that, on one assertion: AssertionError: absolute path D:/shots/out was rewritten to /tmp/ocio_meta_test_xxxx/D:/shots/out check_output_folder asserts that resolve_output_folder leaves an absolute path alone, and offers `D:\shots\out` and `\nas\vfx\out` as the examples. What counts as absolute is os.path.isabs, which answers per platform: both are absolute on Windows and ordinary relative names on Linux. So in the container the resolver was asked about a relative path, correctly joined it to the output directory, and the test called that a failure. The resolver is right; the examples were not portable. Now each platform is asked about paths it considers absolute, with the test asserting isabs on its own example first so this cannot rot silently again. The smoke test that proves the pack imports under ComfyUI listed nine node classes. There are eleven: OCIOVAEDecode and OCIOVAEEncode were never added, so if either failed to register the gate would have said "all 9 OCIO nodes registered" and passed. Added them, and the count in the three messages is now len(OCIO_NODE_CLASSES) rather than a literal, so the next node does not need a docs edit to be covered. Verified: tools/test_write_metadata.py passes on Windows after the change, and the posix examples are absolute under posixpath while the Windows ones are not, which is exactly the container's failure. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 131e5db commit 2a35cb5

4 files changed

Lines changed: 16 additions & 6 deletions

File tree

docker/comfyui_client.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
OCIO_NODE_CLASSES = [
1414
"OCIOColorSpace", "OCIOLogConvert", "OCIODisplay", "OCIOCDLTransform",
1515
"OCIOFileTransform", "OCIOLookTransform", "OCIORead", "OCIOWrite", "OCIOPlayer",
16+
"OCIOVAEDecode", "OCIOVAEEncode",
1617
]
1718

1819

docker/roundtrip_test.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
import urllib.request
3535

3636
sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
37-
from comfyui_client import ComfyUIClient # noqa: E402
37+
from comfyui_client import ComfyUIClient, OCIO_NODE_CLASSES # noqa: E402
3838
from build_workflow import build_graph # noqa: E402
3939
import ocio_names # noqa: E402
4040
import compare_histograms # noqa: E402
@@ -102,7 +102,7 @@ def main():
102102
if not ok:
103103
print(f"[comfyui] OCIO nodes missing from /object_info: {missing}\n" + tail(SERVER_LOG))
104104
return 3
105-
print("[comfyui] all 9 OCIO nodes registered.")
105+
print(f"[comfyui] all {len(OCIO_NODE_CLASSES)} OCIO nodes registered.")
106106

107107
cfg_src = os.environ.get("OCIO") or "(built-in ACES config)"
108108
names = ocio_names.resolve_names()

docker/run_tests.sh

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
#!/usr/bin/env bash
22
# Unit + smoke test entrypoint for the `test` compose service.
33
# 1. Run every standalone tools/test_*.py (exit non-zero on any failure).
4-
# 2. Boot ComfyUI headless and assert all 9 OCIO nodes register (proves the pack imports in ComfyUI).
4+
# 2. Boot ComfyUI headless and assert every OCIO node registers (proves the pack imports in ComfyUI).
55
# 3. Optional: tools/accuracy regression suite when RUN_ACCURACY=1.
66
set -uo pipefail
77

@@ -34,7 +34,7 @@ echo "==================== ComfyUI node-registration smoke ===================="
3434
python - <<'PY'
3535
import subprocess, sys, os
3636
sys.path.insert(0, os.path.join(os.environ.get("OCIO_PACK_DIR", "/opt/ComfyUI/custom_nodes/ComfyUI-OCIO"), "docker"))
37-
from comfyui_client import ComfyUIClient
37+
from comfyui_client import ComfyUIClient, OCIO_NODE_CLASSES
3838
comfy = os.environ.get("COMFYUI_DIR", "/opt/ComfyUI")
3939
log = open("/tmp/comfyui-smoke.log", "wb")
4040
proc = subprocess.Popen([sys.executable, "main.py", "--cpu", "--port", "8188", "--listen", "127.0.0.1"],
@@ -45,7 +45,7 @@ try:
4545
c.wait_until_ready(timeout=300)
4646
ok, missing = c.check_nodes()
4747
if ok:
48-
print(" ok: all 9 OCIO nodes registered in ComfyUI")
48+
print(f" ok: all {len(OCIO_NODE_CLASSES)} OCIO nodes registered in ComfyUI")
4949
else:
5050
print(f" FAILED: missing nodes {missing}")
5151
rc = 1

tools/test_write_metadata.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,16 @@ def check_output_folder(io, tmp):
532532
assert n(r("$OUTPUT/a/b")) == n(os.path.join(tmp, "a", "b")), "a nested token path did not resolve"
533533
assert n(r("shot_010")) == n(os.path.join(tmp, "shot_010")), "a plain relative path changed behaviour"
534534
# An absolute path stays absolute: pointing a Write at a NAS is deliberate, not a mistake to be corrected.
535-
for absolute in (os.path.join("D:" + os.sep, "shots", "out"), r"\\nas\vfx\out"):
535+
# What counts as absolute is `os.path.isabs`, which answers per platform: a drive letter and a UNC share are
536+
# absolute on Windows and plain relative names on Linux. Asking Linux about "D:/shots/out" therefore tests
537+
# that the resolver failed to honour something nobody asked it to, which is how this fails in the container
538+
# while passing on Windows. Same assertion either way, on paths the running platform calls absolute.
539+
if os.name == "nt":
540+
absolutes = (os.path.join("D:" + os.sep, "shots", "out"), r"\\nas\vfx\out")
541+
else:
542+
absolutes = (os.path.join(os.sep, "mnt", "nas", "vfx", "out"), os.path.join(os.sep, "shots", "out"))
543+
for absolute in absolutes:
544+
assert os.path.isabs(absolute), f"the test's own example {absolute} is not absolute on this platform"
536545
assert r(absolute) == absolute, f"absolute path {absolute} was rewritten to {r(absolute)}"
537546
# And the default the node ships with must not be an absolute path in the first place - it is stored in
538547
# widgets_values, and core SaveVideo / SaveImage embed the whole workflow JSON inside the files they write.

0 commit comments

Comments
 (0)