fix(video_compose): surface Remotion failures + add render timeout passthrough (#217)#251
Conversation
calesthio
left a comment
There was a problem hiding this comment.
Requesting changes because remotion_timeout_ms is not actually wired through the high-level path this PR is meant to fix.
The new handling works when _remotion_render() is called directly, but video_compose operation=render goes through _render(), which builds a fresh remotion_inputs dict with only edit_decisions, output_path, and optionally profile before calling _remotion_render(remotion_inputs). As a result, callers using:
VideoCompose().execute({
operation: render,
remotion_timeout_ms: 120000,
...
})never get that timeout passed to the Remotion CLI.
I verified this with a direct probe of execute({operation: render, ..., remotion_timeout_ms: 120000}): a mocked _remotion_render() received no remotion_timeout_ms key. The new tests call _remotion_render() directly, so they miss the high-level forwarding path described in the PR and #217.
Checks run:
python -m pytest tests/tools/test_remotion_diagnostics.py -q-> 4 passedpython -m py_compile tools/video/video_compose.py tests/tools/test_remotion_diagnostics.py-> passed
The stderr/timeout handling inside _remotion_render() looks good; this just needs the high-level render path to forward the option and ideally a test that exercises execute(operation=render) or _render() directly.
…passthrough The high-level Remotion render path hid the useful failure reason. run_command runs with check=True + capture_output, so a non-zero exit raised CalledProcessError whose str() is only 'returned non-zero exit status 1' — the actual Remotion diagnostics in stderr were dropped. Catch CalledProcessError and surface the stderr/stdout tail, and TimeoutExpired with an actionable hint. Also add a creator-facing remotion_timeout_ms input, passed through as Remotion's --timeout (headless-browser setup + delayRender). Slow browser startup on restricted networks previously failed opaquely at the default 30s with no way to raise it. The subprocess timeout is widened to match so run_command does not kill Remotion before its own timeout fires. Closes calesthio#217
…nder path The timeout handling only took effect on a direct _remotion_render() call. The high-level execute(operation='render') path goes through _render(), which builds a fresh remotion_inputs dict (edit_decisions, output_path, profile) and dropped remotion_timeout_ms — so callers of the documented operation='render' path never got the timeout passed to the Remotion CLI. Forward it there. Adds a test exercising _render() (not just _remotion_render()) to cover the high-level forwarding path. Refs calesthio#217
9e9290d to
8c9af32
Compare
|
You're right — the option worked on a direct
Added a test that drives Checks: |
calesthio
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The requested high-level forwarding gap is fixed now: _render() forwards remotion_timeout_ms into the remotion_inputs dict, and the new regression test covers that path.
I verified locally:
python -m pytest tests/tools/test_remotion_diagnostics.py -q-> 5 passedpython -m py_compile tools/video/video_compose.py tests/tools/test_remotion_diagnostics.py-> passed- direct
VideoCompose().execute({"operation": "render", ..., "remotion_timeout_ms": 120000})probe with_remotion_renderstubbed capturedremotion_timeout_ms=120000
GitHub CI is green. I do not have further findings.
…on-debuggable fix(video_compose): surface Remotion failures + add render timeout passthrough (calesthio#217)
Summary
Addresses the debuggability + timeout gaps from #217 in the high-level
video_compose operation=render(Remotion) path.#217 raised three asks; the local-asset staging one is already handled in #238 (assets are staged into
remotion-composer/public/_om_assetsand props rewritten tostaticFile-relative paths). This PR covers the remaining two:Changes
run_commandruns withcheck=True+capture_output, so a non-zero exit raisedCalledProcessErrorwhosestr()is onlyreturned non-zero exit status 1— the useful stderr was dropped (exactly Make video_compose Remotion renders debuggable and local-asset safe #217's complaint README video attachment upload #1). Now caught explicitly and the stderr/stdout tail is included inToolResult.error.TimeoutExpiredgets an actionable message.remotion_timeout_mspassthrough. New optional input, passed through as Remotion's--timeout(governs headless-browser setup +delayRender()). Slow browser startup on restricted networks previously failed opaquely at the default 30s with no override (Make video_compose Remotion renders debuggable and local-asset safe #217 complaint New Pipeline: Talking Head — Turn raw footage into polished social videos #2). The subprocess timeout is widened to match sorun_commanddoesn't kill Remotion before its own timeout fires.Testing
tests/tools/test_remotion_diagnostics.py(new, 4 tests): stderr tail surfaced on failure;TimeoutExpiredmessage namesremotion_timeout_ms;--timeout=<ms>appended and subprocess timeout widened when requested; no flag and 600s default when not.pytest tests/tools/test_remotion_diagnostics.py -q-> 4 passed.character-animationruntime-contract,test_registry_catalog_views) unrelated to this branch.Checklist
Closes #217