Repository navigation
fix(chain): apply --out-file to video chain clips (#1293) - #1297
Merged
Merged
Conversation
video chain accepted --out-file / -o PATH and never read it. run_chain gains an optional relocate hook that moves each downloaded clip before the chain table, the videos catalog, the seed extraction and --json see its path. The CLI numbers links over the whole manifest, so a --resume-from run writes the remaining links under the names a full run would. The stderr notice is removed.
…move it (#1293) A move failure (locked target, full disk) escaped run_chain as an unexpected error before the link reached the chain table, so --resume-from would bill it again. The clip now stays where it downloaded, is recorded at that path, and a warning names the target. Documents --out-file in the CONFIGURATION chain flag table.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
…review) The <stem>_<n><suffix> rule lived in four copies (video, image, worker, chain). paths.numbered_path now holds it, next to relocate_video and relocate_video_output, moved from cli_video. The chain relocator is typed as run_chain declares it, and the chain --out-file test reuses the shared CliRunner helper.
…ed i2v (#1300) --resume-from passed links[skipped:] and run_chain numbered them from 0. The first resumed link then overwrote link 0's chain row (keyed on link_index), went out as an unseeded t2v, and left the row count short, so a later resume billed a finished link again. run_chain takes start_index and seed_clip: request mode, the result and recorded index, --json links[].index and the --out-file number count from the manifest, and the first resumed link is an i2v seeded from the last completed clip. A resume whose clip is gone is refused before the cost prompt with exit 20 naming the file, never sent as t2v.
#1293 review) A second run with the same --out-file moved its clips over the first run's. An existing <stem>_<i><suffix> now keeps the new clip at its download path, recorded there.
review) Any exception from the relocate hook escaped run_chain before the link was recorded, leaving a paid clip without a chain row. The move now runs off the event loop, a failure keeps the clip at its download path, recorded there, with a warning naming the link number, media id and where the clip stayed, and a partial copy the move left is removed.
…anything runs (#1293 review) The clips' folder was first created after the cost prompt and the browser boot, so an --out-file whose parent is a file died as exit 1 with Chrome already up. It is now a usage error (exit 2) up front, in --dry-run too.
The chain e2e calls run_chain directly, so it cannot see --out-file. The new e2e_video test drives video chain over a 2-link manifest and checks clip_1.mp4 and clip_2.mp4 are mp4s, --json reports exactly those paths, and link 2 was recorded as i2v. Collected, not run here: it spends Veo credits.
…xit 20 on resume (#1300) Chain links record command 'video t2v'/'video i2v', so the e2e's filter on 'video chain' matched nothing and would have failed after billing (checked at $0 against a real catalog). The database holds that run only, so the filter is dropped. USAGE now names the refused resume under exit 20 and on --resume-from.
…out-file # Conflicts: # CHANGELOG.md # src/gflow_cli/cli_video.py # src/gflow_cli/paths.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1293. Closes #1300.
What happens today
gflow video chain ... --out-file clip.mp4(or the deprecated-o) exits 0 and writes nothing at that path. Since v0.99.0 it prints a stderr notice; before that the option was a silent no-op.The fix
Clip i, counted in manifest order from 1, is written to
<stem>_<i><suffix>next to PATH (out/clip.mp4givesout/clip_1.mp4,out/clip_2.mp4). This is the naming rule the MCP worker already uses for several outputs.run_chaintakes an optionalrelocatehook that moves each downloaded clip before anything reads its path. So the chain table, the videos catalog, the next link's seed frame, the partial-failure paths and--jsonall report the path actually written.--out-dir. The stderr notice is removed.video chainis exempt in_MCP_EXEMPT, so there is no MCP twin to change. The cloud-storage refusal (video -o/--output, video chain and MCP output_file crash after billing when GFLOW_CLI_STORAGE_URI is a cloud bucket #1008) is unchanged.Tests (each written first and seen failing)
test_chain_out_file_writes_each_link_to_a_numbered_path[--out-file|-o]test_chain_out_file_resume_keeps_each_links_numbertest_chain_out_file_move_failure_keeps_the_billed_clip_recordedFull offline suite: 6892 passed, 34 skipped. All gates are green.
E2E
Pending:
tests/e2e/test_chain_e2e.pywith--out-file. It is paid (two i2v links) and will be run once on the final tree after review.Review (code-review xhigh + ponytail-review xhigh), fixed in one round
Both reviewers ranked one pre-existing bug first, #1300, reproduced on develop with a real recorder.
--resume-fromnumbered the remaining links from 0. That overwrote the chain row of an already-paid link, sent the first resumed link as an unseeded t2v, and billed a link twice on a later resume. With--out-fileit would also have overwritten a paid clip on disk. It is fixed at the root here, because the new file naming depends on the same index:run_chaintakes a start index and the last completed clip. The request index, the recordedlink_index,--jsonlinks[].indexand the file number now all count from the manifest. The first resumed link goes out as i2v, seeded from the last clip. If that clip is gone, the run is refused before submit (exit 20, naming the file).clip_Nfile or folder is never overwritten. The clip stays where it was downloaded, recorded there, with a warning that names the link, the media id and the path.--out-fileunder an existing file is a usage error before anything runs, including under--dry-run.paths.numbered_path) is shared with the image command and the worker, and the relocator is typed.Each fix has a test that was seen failing first. The #1300 test uses a real
ChainLinkRecorderon a temporary database: 3 links, a failure, then two resumes. The offline suite passed: 6900 passed, 34 skipped.E2E
tests/e2e/test_chain_e2e.py::test_chain_cli_out_file_writes_numbered_mp4s_and_seeds_link_2drives the CLI (video chain <2-link manifest> --out-file <tmp>/clip.mp4 --yes --json). The result is recorded below.E2E result (profile served flow.google.com, veo-lite, 20 credits, 636 → 616):
test_chain_cli_out_file_writes_numbered_mp4s_and_seeds_link_2passed.clip_1.mp4andclip_2.mp4were written as mp4s,--jsonreported exactly those paths, and link 2 was recorded as i2v.After the e2e, develop (#1185) was merged in.
cli_video.pythen went past its size baseline, sochain_clip_relocatormoved, verbatim, intopaths.pynext to the helpers it uses, with no logic change. The full offline suite passed afterwards: 6968 passed, 34 skipped. CodeRabbit was rate-limited on this PR and posted no review.