Skip to content

deps: cuda-link 1.12.1 + doorbell persistence fixes#1

Merged
forkni merged 6 commits into
mainfrom
deps/cuda-link-1.12-torch-modelopt-onnx
Jul 14, 2026
Merged

deps: cuda-link 1.12.1 + doorbell persistence fixes#1
forkni merged 6 commits into
mainfrom
deps/cuda-link-1.12-torch-modelopt-onnx

Conversation

@forkni

@forkni forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Bump cuda-link wheel to 1.12.1, auto-persist CUDALINK_LIB_PATH
  • Persist CUDALINK_DOORBELL=1 in phase4c for the doorbell fast path
  • Sort import blocks (ruff I001) in package __init__/__main__
  • Add Claude Code + Copilot automated PR review harness (mirrors the forkni/StreamDiffusion fork setup)

This is PR I-1 of a two-stack installer PR sequence: feat/installer-error-report (PR I-2) chains on this branch's tip since it depends on phase4b_cuda_link/phase4c_cuda_link_env, which exist only here, not on main.

Test plan

  • Claude Code review (requires CLAUDE_CODE_OAUTH_TOKEN secret on this fork)
  • GitHub Copilot review

forkni and others added 4 commits July 12, 2026 16:11
Add Installer.phase4c_cuda_link_env(): after the cuda-link wheel is
installed, persist CUDALINK_LIB_PATH to the user environment via setx
(Windows-only, non-fatal) so TouchDesigner library mode resolves the
venv site-packages with no manual env-var step. Wired into install()
right after phase4b_cuda_link(). Bump the wheel URL and phase progress
string from 1.12.0 to 1.12.1.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
phase4c_cuda_link_env only declared CUDALINK_LIB_PATH, so a fresh
install silently degraded the native-wait IPC path to poll-sleep.
SD's TD topology is bidirectional (TD Sender + SD Exporter are each
a producer on their own IPC leg), and TD's Sender runs in a separate
bundled-Python process that a runtime os.environ.setdefault can't
reach -- the doorbell var must be setx-persisted like CUDALINK_LIB_PATH.
Found via a clean-slate reproducibility test of PR #2's cuda-link
handling (stripped venv package + all cuda-link env vars, then
re-ran phase4b/phase4c to confirm they're self-sufficient).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the installer’s cuda-link dependency and improves TouchDesigner integration by persisting required environment variables, while also adding an automated Claude Code review workflow and repo-level Python review guidance.

Changes:

  • Bump the prebuilt cuda-link wheel URL to 1.12.1 and update the installer progress messaging accordingly.
  • Add a new install phase (phase4c_cuda_link_env) to persist CUDALINK_LIB_PATH and CUDALINK_DOORBELL=1 on Windows via setx.
  • Add a GitHub Actions workflow for Claude Code reviews, and add repo Python review guidance under .github/instructions/.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sd_installer/installer.py Updates cuda-link wheel pin and adds a new phase to persist cuda-link-related env vars for TouchDesigner workflows.
sd_installer/__main__.py Removes trailing whitespace-only line.
sd_installer/__init__.py Removes trailing whitespace-only line.
.github/workflows/claude-code-review.yml Adds an automated Claude Code PR review workflow.
.github/instructions/python.instructions.md Adds repository Python review guidance tailored to this installer.

Comment thread sd_installer/installer.py Outdated
Comment on lines +336 to +346
setx_result = subprocess.run(
["setx", "CUDALINK_LIB_PATH", site_packages],
capture_output=True,
text=True,
)
if setx_result.returncode != 0:
print(f" WARNING: setx failed to persist CUDALINK_LIB_PATH: {setx_result.stderr.strip()}")
else:
print(" CUDALINK_LIB_PATH persisted for this user account.")
print(" Restart TouchDesigner (and any open shells) to pick up the new environment variable.")

Comment thread sd_installer/installer.py Outdated
Comment on lines +353 to +357
db_result = subprocess.run(["setx", "CUDALINK_DOORBELL", "1"], capture_output=True, text=True)
if db_result.returncode != 0:
print(f" WARNING: setx failed to persist CUDALINK_DOORBELL: {db_result.stderr.strip()}")
else:
print(" CUDALINK_DOORBELL=1 persisted (enables doorbell/native-wait IPC fast path).")
Comment on lines +14 to +18
permissions:
contents: read
pull-requests: write
issues: write
id-token: write
The multi-agent code-review plugin exhausted 60 turns on PR #2 (feat/installer-error-report,
a stacked diff with no CLAUDE.md to shortcut convention discovery) before posting a verdict.
Bumping headroom rather than trusting a partial/incomplete review.
@forkni forkni requested a review from CharlieHelps July 14, 2026 02:36
@forkni

forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Re-triggering — the prior review run stalled after the CLAUDE.md-compliance step and never completed the bug scan or posted a final verdict, despite the workflow itself exiting without error.

@forkni forkni closed this Jul 14, 2026
@forkni forkni reopened this Jul 14, 2026
Repository owner deleted a comment from claude Bot Jul 14, 2026
@forkni

forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Deleted the stalled review comment (the precondition check was treating it as a completed review, causing re-runs to short-circuit). Re-triggering for a genuinely fresh review.

@forkni forkni closed this Jul 14, 2026
@forkni forkni reopened this Jul 14, 2026
subprocess.run(['setx', ...]) can raise FileNotFoundError/OSError if setx isn't
resolvable, which propagated unhandled and aborted the install before phases
5-8 ran -- contradicting the method's own 'non-fatal' docstring. Wrap both
calls in try/except OSError, matching the existing returncode!=0 warning path.

Flagged independently by both the Claude and Copilot review passes on PR #1.
Repository owner deleted a comment from claude Bot Jul 14, 2026
@forkni

forkni commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Review stalled again after the fix-commit push (same silent-truncation pattern). Deleted the stale comment and re-triggering once more.

@forkni forkni closed this Jul 14, 2026
@forkni forkni reopened this Jul 14, 2026
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @forkni's task in 3m 10s —— View job


Claude Code Review

Todo list

  • Gather context (PR is open, not draft, no completed prior review)
  • Identify relevant CLAUDE.md files — none exist in this repo, so compliance check is N/A
  • Summarize PR changes
  • Run parallel bug scan (2 independent agents)
  • Validate flagged issues
  • Post final review summary

Summary

This PR (1) bumps the cuda-link wheel pin to 1.12.1, (2) adds a new phase4c_cuda_link_env() install phase that persists CUDALINK_LIB_PATH and CUDALINK_DOORBELL=1 via Windows setx for TouchDesigner integration, (3) applies ruff import-sort formatting to __init__.py/__main__.py, and (4) adds the Claude Code + Copilot PR-review harness (workflow + .github/instructions/python.instructions.md).

Findings

No high-signal issues found.

  • The two Copilot-flagged concerns from the earlier review round — subprocess.run(["setx", ...]) raising OSError/FileNotFoundError and aborting the install despite the "non-fatal" docstring — are already fixed on this branch tip (commit 587ac3e): both setx calls in phase4c_cuda_link_env are now wrapped in try/except OSError, in addition to the existing returncode != 0 check.
  • Two independent bug-scan passes over the diff (logic/control-flow and security/best-effort-contract) turned up nothing that rises to a real bug. Phase ordering (phase4c after phase4b), the Windows-only guard (sys.platform != "win32"), the wheel-URL/progress-message version bump (1.12.0 → 1.12.1, consistent everywhere), and the formatting-only changes in installer.py/__init__.py/__main__.py all check out.
  • One sub-agent noted, as a non-blocking observation, that self._run_python(...) inside phase4c_cuda_link_env (installer.py) isn't itself wrapped in try/except, so a missing/locked venv interpreter could in theory raise and abort the install despite the docstring's "non-fatal" framing. This matches the pre-existing pattern used by other _run_python call sites elsewhere in the file, is extremely unlikely in practice (the venv python is exercised repeatedly by earlier phases before this point), and isn't a regression introduced by this PR — not flagging it as an actionable issue.
  • No CLAUDE.md files exist anywhere in this repository, so there's no compliance check to run there.

No code changes were made as part of this review.

@forkni forkni requested review from Copilot and removed request for CharlieHelps July 14, 2026 02:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@forkni forkni merged commit a0e038c into main Jul 14, 2026
3 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.

2 participants