From b3ed126cd371b79ed167bf9f928ce03ca8eb9498 Mon Sep 17 00:00:00 2001 From: James Veitch Date: Sun, 28 Jun 2026 16:17:13 +0100 Subject: [PATCH] feat(spec): 002-manager-install plan.md + ADR-003 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit plan.md covers: - requirements.txt fix (jinja2 only, hand-authored) - aiohttp→production deps in pyproject.toml - @description metadata update - custom-node-list.json PR scope - Docker Compose CPU-only test harness (US4) ADR-003: hand-authored requirements.txt as curated subset of pyproject.toml — cross-cutting policy for all future dep additions. Co-Authored-By: Claude Sonnet 4.6 --- ...R-003-requirements-txt-authoring-policy.md | 93 +++++++++++ .../Roadmap/epics/ux-and-install.md | 4 +- .../plan.md | 155 ++++++++++++++++++ .../spec.md | 24 +++ 4 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 project-management/ADRs/ADR-003-requirements-txt-authoring-policy.md create mode 100644 specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/plan.md diff --git a/project-management/ADRs/ADR-003-requirements-txt-authoring-policy.md b/project-management/ADRs/ADR-003-requirements-txt-authoring-policy.md new file mode 100644 index 0000000..88a0cb9 --- /dev/null +++ b/project-management/ADRs/ADR-003-requirements-txt-authoring-policy.md @@ -0,0 +1,93 @@ +# ADR-003: Hand-authored requirements.txt as a curated subset of pyproject.toml + +## Status + +> Accepted + +_Date:_ 2026-06-28 +_Deciders:_ darth-veitcher + +--- + +## Context + +comfydv has two distribution paths with different dependency environments: + +**Path A — git-clone (ComfyUI Manager)**: Manager clones the repo into +`ComfyUI/custom_nodes/comfydv/` and runs `pip install -r requirements.txt`. +ComfyUI's own environment already provides `aiohttp` and other packages. + +**Path B — pip install**: `pip install comfydv` in a standalone environment +with no ComfyUI present. All production deps must be declared in +`pyproject.toml [project.dependencies]`. + +The original `requirements.txt` was generated by `uv export --no-editable --no-hashes` +and contained a bare `.` (local editable install), `colorama==0.4.6`, and +`termcolor==2.5.0` — all wrong. The actual runtime dep (`jinja2`) was absent. +Running Manager's post-clone `pip install -r requirements.txt` on this file would fail +or produce a broken environment. + +## Decision + +`requirements.txt` is a **hand-authored**, **human-readable** file committed to git. +It lists only the runtime dependencies that are **not** already provided by ComfyUI's +own environment. It is a curated subset of `pyproject.toml [project.dependencies]`. + +`pyproject.toml [project.dependencies]` remains the **source of truth** for all runtime +dependencies. It includes deps that ComfyUI already provides (e.g. `aiohttp`) for +correctness when the package is installed outside a ComfyUI environment. + +**Rules**: +1. `requirements.txt` is never auto-generated (no `uv export`, no Makefile target). +2. When a new runtime dep is added to `pyproject.toml [project.dependencies]`, the + developer must decide: "Is this already provided by ComfyUI?" If no → add to + `requirements.txt` too. If yes → leave `requirements.txt` unchanged. +3. A test in `tests/test_packaging.py` asserts that `requirements.txt` contains no + packages absent from `pyproject.toml [project.dependencies]` (no phantom deps). + +## Consequences + +**Easier:** +- ComfyUI Manager install succeeds with a clean `pip install -r requirements.txt`. +- `requirements.txt` is readable and reviewable by humans. +- The policy is explicit: new contributors know what to do when adding a dep. + +**Harder / constrained:** +- Adding a new runtime dep requires updating two files and making a judgment call about + which is ComfyUI-provided. This is a one-minute decision but must not be forgotten. +- The test in `test_packaging.py` catches only phantom deps (things in `requirements.txt` + not in `pyproject.toml`); it does not catch missing deps (things in `pyproject.toml` + that should be in `requirements.txt` but aren't), because that requires knowing which + packages ComfyUI provides. + +**Debt introduced:** +- None beyond the manual sync obligation above. + +## Considered Alternatives + +### Alternative A: Auto-generate `requirements.txt` from `pyproject.toml` via `uv export` + +**Why rejected:** `uv export` produces locked, pinned versions (`colorama==0.4.6`) — +the wrong format for a plugin that must coexist with whatever ComfyUI has installed. +It also includes the local package itself (`.`) and transitive deps of removed packages, +as demonstrated by the original broken file. + +### Alternative B: Remove `requirements.txt` entirely; rely on `pyproject.toml` + +**Why rejected:** ComfyUI Manager's git-clone path explicitly looks for and runs +`pip install -r requirements.txt`. Without it, runtime deps (e.g. `jinja2`) would not +be installed for git-clone users, and FormatString would fail on first use. + +### Alternative C: List all deps (including ComfyUI-provided) in `requirements.txt` + +**Why rejected:** If Manager's `pip install -r requirements.txt` installs a different +version of `aiohttp` than ComfyUI expects, it can silently downgrade ComfyUI's web +server. The minimal-subset approach avoids this conflict entirely. + +--- + +## Links + +- Related spec: `specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/` +- Parent epic: `project-management/Roadmap/epics/ux-and-install.md` +- Related ADRs: [ADR-001](ADR-001-stdlib-logging-over-console-libraries.md) diff --git a/project-management/Roadmap/epics/ux-and-install.md b/project-management/Roadmap/epics/ux-and-install.md index dc06142..8b9694f 100644 --- a/project-management/Roadmap/epics/ux-and-install.md +++ b/project-management/Roadmap/epics/ux-and-install.md @@ -27,9 +27,7 @@ _None — this epic is self-contained and can start immediately._ - specs/005-metadata-cleanup-pyproject-description-scripts-dead-code-removal-tooltips-category-consistency/ ## ADRs -_To be recorded as cross-cutting decisions are made during DESIGN._ - - +- [ADR-003](../../ADRs/ADR-003-requirements-txt-authoring-policy.md) — hand-authored requirements.txt as curated subset of pyproject.toml ## Success criteria diff --git a/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/plan.md b/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/plan.md new file mode 100644 index 0000000..5d676d5 --- /dev/null +++ b/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/plan.md @@ -0,0 +1,155 @@ +# Implementation Plan: Manager-Compatible Install + +**Branch**: `002-manager-install` | **Date**: 2026-06-28 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/spec.md` + +## Summary + +Fix comfydv's packaging so ComfyUI Manager can install it correctly: replace the broken +auto-generated `requirements.txt` with a hand-authored one listing only `jinja2>=3.1.6`, +promote `aiohttp` from dev-only to production dependencies in `pyproject.toml`, correct the +stale `@description` metadata in `__init__.py`, and submit a PR to +`ltdrdata/ComfyUI-Manager` to list the package in Manager's search UI. + +## Technical Context + +**Language/Version**: Python 3.11 (`pyproject.toml` target), plain text (`requirements.txt`), JSON (`custom-node-list.json` PR) + +**Primary Dependencies**: +- `jinja2>=3.1.6` — sole runtime dep not bundled by ComfyUI +- `aiohttp>=3.9.0` — provided by ComfyUI's own environment; must be listed in `pyproject.toml [project.dependencies]` for pip-install correctness but **not** in `requirements.txt` (ComfyUI already satisfies it for git-clone users) + +**Storage**: N/A — no data storage changes + +**Testing**: `uv run pytest` — new test asserting `requirements.txt` correctness and `@description` accuracy + +**Target Platform**: Any environment where ComfyUI is installed; CI (GitHub Actions / local) + +**Project Type**: ComfyUI custom node plugin; packaging/configuration spec + +**Performance Goals**: N/A — install is a one-time operation + +**Constraints**: `requirements.txt` must be parseable by `pip install -r`; the ComfyUI Manager PR must follow the exact JSON schema of `custom-node-list.json` + +**Scale/Scope**: 3 files changed, 1 external PR opened; no new source files + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-checked after Phase 1 design.* + +| Principle | Status | Notes | +|---|---|---| +| I. ComfyUI Contract First | ✅ Pass | No changes to node registration or `NODE_CLASS_MAPPINGS` | +| II. Sandbox All User-Supplied Code | ✅ Pass | N/A — no user-input evaluation changes | +| III. Test-First (NON-NEGOTIABLE) | ✅ Pass | Tests written before implementation (TDD pairs in tasks.md) | +| IV. Graceful Degradation Outside ComfyUI | ✅ Pass | Fix improves this — correct deps mean clean import outside ComfyUI | +| V. Simplicity — Function Before Class | ✅ Pass | Pure configuration changes; no new abstractions | +| VI. Fixed Output Positions | ✅ Pass | N/A — no output schema changes | + +No violations. No Complexity Tracking table needed. + +## Project Structure + +### Documentation (this feature) + +```text +specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/ +├── spec.md ✅ done +├── plan.md ✅ this file +├── research.md ✅ inlined below (no open questions) +└── tasks.md ← /beacon:tasks next +``` + +### Source Code (repository root) + +```text +requirements.txt ← rewrite (FR-001, FR-002) +pyproject.toml ← move aiohttp to [project.dependencies] (FR-003) +__init__.py ← update @description (FR-005) +tests/test_packaging.py ← new test file (constitution III) +``` + +External artefact (not in this repo): +``` +ltdrdata/ComfyUI-Manager / custom-node-list.json ← PR (FR-004) +``` + +## Research + +### Decision 1: What belongs in `requirements.txt` vs `pyproject.toml` + +**Context**: Two separate distribution paths exist: git-clone (ComfyUI Manager) and pip install. They have different dependency environments. + +**Decision**: `requirements.txt` lists only deps **not already provided by ComfyUI's own environment**. ComfyUI depends on `aiohttp` itself, so it is always present for git-clone users. Only `jinja2>=3.1.6` goes in `requirements.txt`. + +`pyproject.toml [project.dependencies]` lists **all** production runtime deps including `aiohttp>=3.9.0`, for correctness when the package is pip-installed outside a ComfyUI environment. + +**Rationale**: Avoids version conflicts from double-installing `aiohttp` in Manager's pip step; keeps `requirements.txt` minimal and correct. + +**Alternatives considered**: +- List `aiohttp` in `requirements.txt` too → redundant, and if versions conflict with ComfyUI's pinned version, Manager install silently downgrades ComfyUI's server +- Remove `aiohttp` from `pyproject.toml` entirely → correct for git-clone but breaks `pip install comfydv` in standalone environments + +### Decision 2: `aiohttp` version constraint + +**Decision**: `aiohttp>=3.9.0` — matches the floor ComfyUI itself uses. No upper bound (semver convention for libraries). + +**Alternatives considered**: Pinning to `==3.9.x` would prevent upgrades when ComfyUI upgrades its own aiohttp. + +### Decision 3: `custom-node-list.json` entry format + +**Decision**: Follow the exact schema used by existing entries. Required fields: +```json +{ + "author": "Darth Veitcher", + "title": "Comfy DV Nodes", + "reference": "https://github.com/darth-veitcher/comfydv", + "files": ["https://github.com/darth-veitcher/comfydv"], + "install_type": "git-clone", + "description": "...", + "nodename": ["Format String (Python f-strings)", "Random Choice", "Circuit Breaker"] +} +``` + +**Rationale**: Manager parses this exact structure. `install_type: "git-clone"` is the standard path; `files` contains the repo URL for Manager to clone. + +### Decision 4: `@description` replacement text + +**Decision**: Replace the stale description with one that names only the three existing nodes and their purpose, removing the "model memory management" / "Model Unloader" reference. + +**New text**: `"Quality of life ComfyUI nodes: dynamic string formatting with Python f-strings or Jinja2 templates, seed-controlled random input selection, and workflow circuit-breaker for conditional queue interruption."` + +## Contracts + +No external API contracts to define — this spec touches only packaging metadata and a +third-party JSON registry. The `requirements.txt` format is dictated by pip (no new +contract to author). + +## Docker Compose Test Harness + +A CPU-only ComfyUI environment is needed to verify SC-001 (deps install cleanly) +and SC-003 (nodes appear in the menu after install) without a GPU. This is deliverable +alongside the packaging fixes in this spec. + +**Approach**: `docker-compose.yml` at the repo root with a single `comfyui` service: +- Base image: `python:3.11-slim` (or a community ComfyUI CPU image if one exists and is stable) +- Startup: clone / install ComfyUI CPU-only, then symlink/copy `comfydv` into + `custom_nodes/`, run `pip install -r requirements.txt`, start ComfyUI server +- No GPU required: `--cpu-only` flag or `torch` CPU wheel +- Health check: `curl http://localhost:8188/` returns 200 → nodes loaded successfully +- Used by: developers testing install changes locally; future CI smoke test + +**Constraints**: The harness must start in under 3 minutes on a developer laptop +(no model downloads; only the ComfyUI server + custom node registration). Model weights +are intentionally excluded — the goal is testing node registration and import, not +inference. + +This is tracked as a dedicated task pair (T050-T / T050-I) in tasks.md. + +## ADR Candidates + +The following decision is cross-cutting (applies to all future specs, not just this one) +and warrants an ADR in the parent epic: + +- **`requirements.txt` authoring policy**: hand-authored subset of `pyproject.toml` deps excluding ComfyUI-provided packages. This policy applies to every future PR that adds a runtime dependency. Recommend adding as `ADR-003` before closing this spec. diff --git a/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/spec.md b/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/spec.md index 9fb211a..83e63d0 100644 --- a/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/spec.md +++ b/specs/002-manager-compatible-install-requirements-txt-and-custom-node-list-registration/spec.md @@ -72,6 +72,30 @@ nodes. --- +### User Story 4 — Local test harness for install verification (Priority: P2) + +A developer working on packaging or node registration can spin up a CPU-only ComfyUI +environment locally in one command to verify that dependencies install cleanly and nodes +appear in the menu — without needing a GPU or a full ComfyUI installation on their +machine. + +**Why this priority**: Without a local harness, SC-001 and SC-003 can only be verified +by manually installing ComfyUI on a development machine, which is slow and environment- +dependent. A Compose harness makes these criteria continuously verifiable and sets the +foundation for future CI smoke tests. + +**Independent Test**: `docker compose up --build` from the repo root starts a ComfyUI +server, installs comfydv via `requirements.txt`, and the server health endpoint returns +200 with no import errors in the logs. + +**Acceptance Scenarios**: + +1. **Given** a machine with Docker and no GPU, **when** `docker compose up --build` is run from the repo root, **then** the ComfyUI server starts and `curl http://localhost:8188/` returns 200. +2. **Given** the running container, **when** the ComfyUI startup logs are inspected, **then** all three comfydv nodes (FormatString, RandomChoice, CircuitBreaker) appear as loaded with no import errors. +3. **Given** the running container, **when** `docker compose down` is run, **then** it exits cleanly with no lingering processes. + +--- + ### Edge Cases - What if `jinja2` is already installed? `pip install -r requirements.txt` is idempotent — it satisfies the constraint without downgrading or reinstalling.