feat(ai): complete the prompt loop — real ffprobe metadata on accept, clip on the timeline, Export wired to render jobs - #91
Conversation
Task 5 — boundary: add an app.rfir bridge package in the orchestrator that grafts model-workers' RFIR sources onto the orchestrator's own package search path (path dependency on model-workers). Both services keep their top-level 'app' package; the bridge sidesteps the name collision because the RFIR subtree only imports within app.rfir.* (guarded by a new test). Production GPU execution still flows through Redis to model-workers. Task 6 — implement compile_and_run_tier_a() in cfsv_pipeline: compiles, validates, executes the Tier-A graph in-process and returns output.mp4, keyframe PNGs, graph.json, and executor metrics as an ok-JSON contract. Task 7 — worker_loop: when RENDERFLOW_RFIR_ENABLED=true and a scene job is processed locally (no Redis dispatch), run the real RFIR pipeline instead of run_scene_stages() stubs; metadata.output_path points at the real MP4. Task 8 — RFIR-aware stages and failure handling: run_graph() gains an on_node_start hook; the worker maps graph ops to stages (compiling, generating_keyframe, estimating_depth, rendering_frames, muxing), honors mid-run cancellation, and lands failures in FAILED with metadata.error. Supporting changes: torch imports in ltc/vae/sparse_t2v_window become lazy (segment_subject's was unused) so the executor imports with only numpy+pillow — both now orchestrator deps; preview endpoint 503 contract kept for deployments without the monorepo checkout.
Boots the real FastAPI app (lifespan, worker thread) via TestClient and drives the acceptance path over HTTP: POST /v1/jobs mode=scene -> guardrails -> worker -> RFIR compile/execute -> ffmpeg mux -> GET shows RFIR stages and review with metadata.output_path at a real MP4, then accept commits a video asset. The failure path runs with zero fakes: torch isn't installed, so inference genuinely fails and the job must land in failed with metadata.error. Also covers /v1/rfir/preview end to end. Adds httpx as a dev dependency (required by starlette's TestClient). Verified additionally with a live uvicorn smoke run: health ok, preview compiles through the bridge, and a submitted scene job progressed preparing > compiling > generating_keyframe > failed with "No module named 'diffusers'" — the designed no-ML-runtime behavior.
- Hoist graph validation into _build_tier_a (raises CompileError), removing the duplicated validate-and-return block from compile_tier_a and compile_and_run_tier_a. - Move the op->stage mapping into renderflow_queue.job_status as RFIR_OP_STAGES / stage_for_op(): stage vocabulary is a cross-service contract, and keeping it in the shared lib stops the in-process and Redis execution paths from drifting apart as ops evolve. - Add tests/conftest.py with the shared _no_db and fake_ml_ops fixtures and the expected RFIR stage progression, replacing three near-identical copies across the new test files. - Log a warning from the app.rfir bridge when it cannot locate the RFIR sources instead of failing silently at first import. - Reuse a single stages snapshot per update in _process_scene_job_rfir.
…t-loop # Conflicts: # lib/renderflow_queue/renderflow_queue/__init__.py # services/model-workers/app/rfir/executor/engine.py # services/orchestrator/.env.example # services/orchestrator/app/media/cfsv_pipeline.py # services/orchestrator/app/worker_loop.py # services/orchestrator/tests/test_cfsv_pipeline.py # services/orchestrator/tests/test_rfir_bridge.py # services/orchestrator/tests/test_worker_loop_rfir_inprocess.py
…adata accept_ai_job stamped every AI asset with duration_ms=10_000 and 1920x1080 regardless of what RFIR actually rendered. The UI derives clip length from duration_ms and shows width/height/fps in the asset list, so a 5s 512x288 render became a 10s 1080p clip on the timeline. Reuse ffmpeg_util.detect_format() — the same probe the media import path already uses — and build the asset meta from it. When ffprobe is missing or the file is unreadable, record probe_error and leave duration/size unknown rather than inventing numbers a wrong-length clip would follow. Also drops the no-op proxy thread (its target was `pass`, and the AI mp4 is already its own proxy). It forced tests to patch threading.Thread, which patches the shared module and broke subprocess inside ffprobe.
…o render jobs Accepting a reviewed job only refreshed the asset list, so the generated clip never reached the timeline, and Export logged "not yet wired". ops/aiAccept.ts fetches the accepted asset, inserts it via the shared video-track path, and persists the clip against the sequence's video track (creating one if the sequence has none). That server copy is the part that matters: render jobs build the export from the server's clips, so a browser-only clip would never appear in the exported file. ops/export.ts submits a render job, polls it to completion, and reports progress. A failed render comes back as a job carrying its error rather than an exception; a wedged render gives up instead of polling forever. The result lands in a status readout next to the button, since devLog only prints in dev mode. insertAssetIntoVideoTrack is extracted from the two copies of that block in the import and drag-drop handlers, so import, drop, and accept now share one insertion path. test_ai_prompt_loop.py walks the whole loop over HTTP: accept a job with a real 2s artifact, put its asset on the sequence, render, and assert the exported file is that clip (2s) rather than the 5s empty-sequence placeholder.
…rectly
insertClipFromAsset built the label with uri.split('/'), which never
splits a Windows path, so an imported or accepted clip read as the full
C:\... path on the timeline. Prefer the asset's display name (AI clips
carry their prompt there) and split on both separators otherwise.
Restores dev's bytes in .env.example and cfsv_pipeline.py so this branch carries no unrelated churn.
|
/sorge |
Sorge AI Code ReviewModel: gemini-2.5-flash (gemini) SummaryThis PR introduces robust AI job acceptance and export capabilities to the Studio application. Key improvements include new UI modules (aiAccept.ts, export.ts) that encapsulate these operations with good testability, and a refactoring in studioApp.ts to consolidate asset insertion logic into insertAssetIntoVideoTrack. On the backend, the accept_ai_job endpoint in the Orchestrator now accurately extracts media metadata (duration, dimensions, codec) from AI-generated outputs using ffprobe, replacing previously hardcoded values. This is a significant enhancement for data integrity and UI accuracy. The addition of comprehensive unit and end-to-end integration tests for both frontend and backend components is commendable, ensuring the reliability of these new features. One critical import is missing in the backend, which needs to be added to prevent runtime errors. Issues Foundℹ️ apps/desktop-tauri/ui/src/studioApp.ts:1030
Recommendations
Routing (scheduled, 1 chunk(s))
Review generated by deepiri-sorge |
|
@PeytonLi fix what the sorge said please sir |
ElgineTham
left a comment
ElgineTham
left a comment
There was a problem hiding this comment.
Everything looks good, however the fix is still only half done. In doAcceptJob, if insertAcceptedClip puts the clip on the local timeline and then createClip fails, the catch only calls devLog. The status readout stays at Status: accepted. devLog is hidden in production, so the user never learns that Export will miss the clip (render jobs use list_clips_for_sequence, not the local timeline).
After this change is made, request me again for review and I'll approve!
createClip failing after insertAcceptedClip put the clip on the local timeline only reached devLog, which is hidden in production. The status readout stayed at 'Status: accepted' and Export (which renders the server's list_clips_for_sequence) would silently skip the clip. insertAcceptedClip now throws ServerSyncError carrying the clip, and doAcceptJob surfaces it in the UI: the readout names the clip and says the server copy is missing so Export will skip it. Other insert failures and the no-asset case get explicit messages too.
|
Fixed the review comments — @ElgineTham, please re-review.
Verified: 103/103 UI tests (2 new/strengthened around ServerSyncError), |
4 participants
Closes the AI prompt loop: prompt → generate → accept → clip on the timeline → export.
Three gaps were breaking that loop.
1. Accept stamped hardcoded metadata
accept_ai_jobcreated every AI asset withduration_ms=10_000and1920x1080, regardless of what RFIR actually rendered. The UI derives clip length fromduration_msand shows width/height/fps in the asset list, so a 5s 512x288 render landed as a 10s 1080p clip.It now probes the artifact with
ffmpeg_util.detect_format()— the same helperimport_assetalready used — and builds the asset meta from the result. When ffprobe is missing or the file is unreadable, it recordsprobe_errorand leaves duration/size unset rather than inventing numbers that become a wrong-length clip.Also drops the no-op proxy thread (its target was
pass, and the generated MP4 is already its own proxy). It forced tests to patchthreading.Thread, which patches the shared module and brokesubprocessinside ffprobe.2. Accepting a job never put the clip on the timeline
Accept only refreshed the asset list.
ops/aiAccept.tsnow fetches the accepted asset, inserts it via the shared video-track path, and persists the clip against the sequence's video track, creating one if the sequence has none.That server-side write is the part that matters: render jobs build the export from
list_clips_for_sequence, so a browser-only clip would never reach the exported file.orchestratorCreateClipalready existed inbackendApi.tsand had zero callers before this.3. Export logged "not yet wired"
ops/export.tssubmits a render job, polls it to completion, and reports progress. A failed render comes back as a job carrying itserrorrather than an exception; a wedged render gives up instead of polling forever. The result lands in a status readout next to the button, sincedevLogonly prints in dev mode.Along the way
insertAssetIntoVideoTrackis extracted from the two copies of that block in the import and drag-drop handlers, so import, drop, and accept share one insertion path.uri.split("/"), which never splits a Windows path — clips read as the fullC:\...path on the timeline. Now prefers the asset's display name (AI clips carry their prompt there) and splits on both separators.Verification
tsc --noEmitclean.test_ai_prompt_loop.pywalks the whole loop over HTTP: accept a job with a real 2s artifact → asset → clip on the sequence → render → assert the exported file is 2s. A companion test pins the empty-sequence placeholder at 5s, so the main assertion can't pass vacuously.export.mp4(5s, h264, 1920x1080) with the path shown in the UI; import showed0:05 1920x1080 25.00 fpsfrom the real probe and put a correctly-sized, correctly-labelled clip on V1.Known gaps (not addressed here)
app/media/render_worker.py::render_sequenceis trim/gap-aware, tested, and still not wired. The live API path (app/render_worker.py) does a naive concat that ignores clip in/out points. Export works; trims won't be honored until that swap happens. Best next step.UiTracks onopenProject.Merge note
Merging
origin/devproduced 8 conflicts (the guardrails/CFSV work), resolved keeping both sides. One casualty surfaced as a test failure and is fixed: a droppedfrom pathlib import Pathinworker_loop.py. Theimport subprocess, shutilthatdevcarries there is genuinely dead now that RFIR runs in-process — verified no remaining references.