windows support omnidrive#321
Conversation
Greptile SummaryThis PR adds Windows support to the omnidrive integration across three modules: optional
Confidence Score: 5/5Safe to merge — the changes are well-scoped Windows stubs and defensive guards that don't alter existing POSIX behavior. All three changed modules have correct OS guards: the fcntl optional-import pattern is sound, the entry-point short-circuits prevent any unreachable ioctl path from being accidentally exercised, the compiler/linker flag split mirrors the existing POSIX structure, and the getattr cleanup fix is a standard defensive pattern. A new test covers the Windows fcntl-absent code paths. No existing logic is modified. The Windows linker path in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_get_plugin called] --> B{os.name == 'nt'?}
B -- Yes --> C[find_cl_path / ensure cl.exe in PATH]
C --> D[cc_opts = MSVC flags /wd4067 /wd4624]
D --> E[cuda_opts = nvcc flags no -Xcompiler -Wall]
E --> F[_cuda_home from CUDA_HOME or CUDA_PATH]
F --> G{_cuda_home set?}
G -- Yes --> H[ldflags = cuda.lib + /LIBPATH]
G -- No --> I[ldflags = cuda.lib only]
B -- No --> J[cc_opts = GCC/Clang -Wall -Werror -Wno-psabi]
J --> K[cuda_opts with -Xcompiler -Wall,-Werror,-Wno-psabi]
K --> L[ldflags = -lcuda]
H --> M[cpp_extension.load]
I --> M
L --> M
Reviews (3): Last reviewed commit: "Address PR #321 review: Windows fcntl gu..." | Re-trigger Greptile |
| elif os.name == "nt": | ||
| # Link the CUDA driver API (cu* functions). MSVC needs cuda.lib, which | ||
| # lives in $CUDA_HOME\lib\x64 (not auto-added like the runtime libs). | ||
| _cuda_home = os.environ.get("CUDA_HOME") or os.environ.get("CUDA_PATH") or "" | ||
| ldflags = ["cuda.lib"] | ||
| if _cuda_home: | ||
| ldflags.append("/LIBPATH:" + os.path.join(_cuda_home, "lib", "x64")) |
There was a problem hiding this comment.
cuda.lib added without a search path when env vars are unset
When both CUDA_HOME and CUDA_PATH are absent, ldflags is set to just ["cuda.lib"] with no /LIBPATH: directive. The comment notes that cuda.lib is "not auto-added like the runtime libs", so the MSVC linker will only find it if it happens to be in a default search path (e.g. via the Visual Studio CUDA integration). In a CI/container environment without VS integration this silently produces a link error. Emitting a warning or raising an error when _cuda_home is empty would make the failure actionable rather than cryptic.
|
/ok to test fcfe4e4 |
|
Thank you for your contribution ! |
Please add a concise MR description , including which Windows version and HW tested |
|
assisted with claude Review — PR #321 "windows support omnidrive"Verdict: LGTM minor suggestions. I checked out
Per-file notes
|
| Config | Native Windows (this PR) | WSL2 (same rig) |
|---|---|---|
| Default manifest (1280×704, 2 steps) | 7.3 FPS (8-frame chunk ≈ 1035 ms, model 1007 ms; peak 21.9 GiB) | ~6.8 FPS |
Perf manifest w/ native_dit_acceleration: disabled (1168×640, compile_net) |
13.4 FPS (chunk ≈ 539 ms; peak 20.0 GiB) | — |
| Perf manifest, full native FP8/cuDNN DiT | does not build on Windows (see below) | ~15–20 FPS |
Native Windows matches (slightly beats) WSL2 at identical config, fits comfortably in 32 GB with --offload-text-encoder, and the browser drive loop works end to end.
Remaining gap to Linux is entirely the omnidreams_singleview native extension, which is out of this PR's scope but worth tracking as follow-ups (happy to file issues):
sync_thirdparty.pyfails on Windows checkouts withcore.autocrlf=true: the*.patchfiles get CRLF-translated andgit applyreports "corrupt patch". A.gitattributeswith*.patch -text(repo has none today) fixes it at the root; I worked around it withGIT_CONFIG_*env overrides.omnidreams/native/omnidreams_singleview.pycuDNN discovery is Linux-only (libcudnn.so.9checks, no Windows cudnn lookup), so the build dies atcudnn.h, and the-lcublas/-l:libcudnn.so.9ldflags are GCC-style.- Sage3/Sparge are already explicitly reported unsupported on win32 (
optimized_dit.py:860,886) — the cuDNN attention backend would be the natural first Windows target.
Other run notes: Triton autotune rejects the 128–144 KB smem kernel candidates on consumer Blackwell (benign "out of resource" warnings, same as Linux); triton-windows 3.5 handled all Inductor kernels without issue.
Request-changes-level items
None. The fcntl comment wording is the only thing I'd fix before merge (a one-liner), plus please add a PR description covering the tested toolchain and the Sage3/Sparge scope limit — the diff is small but it's a platform-support claim, and the next Windows user will want to know what was validated ("win" as the commit message undersells a working native port!).
Nice work — small diff that's the missing piece for native Windows; everything else (cu130 index pin for win32, triton-windows, TE exclusion) was already in main and holds up under a real run.
Guard the evdev entry points (read_evdev_name, scan_evdev_devices) when fcntl is unavailable (Windows) so the unreachable ioctl paths are never entered, and reword the comment to match. Add a regression test. Tidy the Windows renderer-plugin branch: drop the unused lib_dir, add a /W3 /WX TODO, and note that torch _nt_quote_args keeps the /LIBPATH intact for space-containing CUDA paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
My Claude did: |
|
thanks for revision ! Approved , testing next. |
No description provided.