From 3756c60ca0fcd8dd4d236a4edb3958e951f06eda Mon Sep 17 00:00:00 2001 From: SolitaryThinker Date: Mon, 4 May 2026 13:00:20 -0700 Subject: [PATCH] [docs] dreamverse-integration: track GpuPool architecture decision (D-12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds D-12 to decisions-log.md, documenting the Oracle review of whether GpuPool (PR #1257, just merged) should be folded into VideoGenerator. Decision summary: keep GpuPool separate from VideoGenerator (Alt A) as interim, evolve to Alt C (thin async executor over generate_async) once PR 7.10 lands. Do not pursue Alt B (folding into VideoGenerator). Captures: * Three alternatives evaluated (A: status quo, B: VideoGenerator absorbs pool role, C: async executor over generate_async) * Key finding: MultiprocExecutor and SubprocessGpuPool are orthogonal, not redundant — both spawn subprocesses because CUDA contexts demand process boundaries, but they solve different problems (one-call-fast vs. many-sessions-throughput) * Sticky binding stays in pool, NOT in VideoGenerator (different consumers want different policies — sticky for streaming, lease for HTTP, queue for per-frame real-time) * Risks flagged: GpuPool.run() sync shape, PoolAssignment.gpu_id: int freeze, and the 'is this canonical serving API?' framing question Action items deferred to PR 7.10 cycle: replace run() with run_async() returning AsyncIterator[VideoEvent], clarify worker_id vs gpu_id field naming, mark GpuPool docstring as experimental until 7.10. --- .../dreamverse-integration/decisions-log.md | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/.agents/memory/dreamverse-integration/decisions-log.md b/.agents/memory/dreamverse-integration/decisions-log.md index 6a460e906..3de9ba7aa 100644 --- a/.agents/memory/dreamverse-integration/decisions-log.md +++ b/.agents/memory/dreamverse-integration/decisions-log.md @@ -6,7 +6,7 @@ question/decision, rationale, current status. For implementation status see [pr-roadmap.md](pr-roadmap.md). For follow-up actions see [open-threads.md](open-threads.md). -**Last updated:** 2026-05-03. +**Last updated:** 2026-05-04 (added D-12 — GpuPool layer separation, Oracle review post-#1257-merge). ## Status legend @@ -14,6 +14,79 @@ follow-up actions see [open-threads.md](open-threads.md). - 🟡 **Deferred** — decision made, implementation deferred to a known PR - 🔴 **Open** — needs decision +## Post-merge architecture decisions + +### D-12: `GpuPool` layer separation — keep distinct from `VideoGenerator` + +**Status:** ✅ Resolved (interim) + 🟡 Deferred long-term shape to PR 7.10. +**Source:** Oracle review on 2026-05-04, post-PR-#1257 merge. + +**Question:** Should `fastvideo.entrypoints.streaming.GpuPool` (PR #1257) be +folded into `fastvideo.entrypoints.video_generator.VideoGenerator`, or kept +separate? Three alternatives were evaluated: + +| Alt | Approach | Verdict | +|---|---|---| +| A | Status quo — `VideoGenerator` (single inference call) and `GpuPool` (multi-session orchestration) stay separate | ✅ Correct as **interim** | +| B | `VideoGenerator` absorbs the pool's role (`from_pretrained_pool`, `acquire/release/run`) | ❌ **Wrong layer.** Conflates execution with serving scheduler. | +| C | `GpuPool` becomes a thin **session-aware async executor** over PR 7.10's `generate_async` | ✅ Correct **long-term destination** | + +**Decision:** Alt A as interim; evolve toward Alt C once PR 7.10 lands +`generate_async`. Do NOT pursue Alt B. + +**Rationale:** + +- `VideoGenerator` is a library handle — "execute one request, possibly + across ranks via `MultiprocExecutor`/`RayDistributedExecutor`." +- `GpuPool` is serving infrastructure — "schedule N concurrent sessions + across N independent replicas, with sticky session-to-GPU affinity for + cache locality." +- These are different layers driven by different consumers (a Python + script doing `gen.generate(req)` vs. a WebSocket server with sticky + sessions). Folding them muddies both surfaces. + +**Key finding — `MultiprocExecutor` and `SubprocessGpuPool` are orthogonal, +not redundant:** + +| Layer | Job | Granularity | +|---|---|---| +| `MultiprocExecutor` (`fastvideo/worker/`) | TP/SP shard ONE inference call across N GPU ranks | per-call | +| `streaming_generator.py` (existing real-time path) | Per-frame streaming via `MultiprocExecutor.submit_step`/`get_result` | per-step within one generator | +| `SubprocessGpuPool` (`entrypoints/streaming/`, PR #1257) | Serve N concurrent sessions on N replicas, sticky-bound | per-session | + +Both spawn subprocesses because **CUDA contexts demand process boundaries**, +not because they solve the same problem. Sharing low-level lifecycle +utilities (process spawn, queue plumbing, shutdown) is a future refactor; +unifying the abstractions is wrong. + +**Sticky binding stays in the pool, NOT in `VideoGenerator`:** sticky +session-to-GPU affinity is a serving policy driven by LTX-2's per-GPU +continuation cache (last-9-decoded-frames + audio-latents). Different +consumers want different policies — stateless OpenAI HTTP wants +per-request leasing; LTX-2 streaming wants sticky affinity; per-frame +real-time streaming wants a continuous queue. Keeping policy in the pool +keeps `VideoGenerator` policy-free. + +**Specific risks flagged in PR #1257 (already merged):** + +| Risk | Mitigation (when relevant) | +|---|---| +| `GpuPool.run() -> Any` is sync — fine for whole-segment dispatch, blocks on cancellation | Replace with `run_async() -> AsyncIterator[VideoEvent]` in PR 7.10 cycle (`generate_async` makes this trivial) | +| `PoolAssignment.gpu_id: int` assumes one-GPU-per-worker | Don't lock as public API. Future may need `device_ids: list[int]` for topology-aware pooling (one worker = group of GPUs running internal `MultiprocExecutor`) | +| `GpuPool` could be documented as the canonical FastVideo serving API | Mark as **experimental / server-internal** in docstring until PR 7.10 lands. Don't include in user-facing API docs yet | +| Memory: N processes = N model replicas (~10-50 GB each) | Expected for concurrent serving with crash isolation. CUDA IPC weight sharing loses isolation; CPU-shared-memory loading helps host RAM not device. Real scalable path is topology-aware pooling later. | + +**Action items (carried into post-7.10 cycle):** + +- [ ] Update `GpuPool` ABC docstring to note "API may change post-PR-7.10" +- [ ] Plan to replace `run()` with `run_async() -> AsyncIterator[VideoEvent]` in PR 7.10 cycle +- [ ] Don't promote `gpu_id: int` to public API; revisit shape post-7.10 +- [ ] Consider clarifying field naming (e.g. `worker_id` is the stable identifier; `gpu_id` is current-impl detail) +- [ ] When opening 7.10's PR, have it consume `generate_async` from `GpuPool.run_async` end-to-end + +**Open thread it touches:** PR 7.10 (`open-threads.md` item D — generate_async) +unblocks Alt C and is the natural place to land the API shape change. + ## D-decisions (from `dreamverse_review.md`, Apr 26) ### D-1: Realtime runtime → streaming GpuPool migration shape