Repository navigation
[bugfix] Fix MPS worker handoff and named Studio image downloads - #1847
leo2ultimate wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Welcome to FastVideo! Thanks for your first pull request.
How our CI works:
PRs run a three-tier CI system:
- Pre-commit — formatting (yapf), linting (ruff), type checking (mypy). Runs immediately on every PR.
- Fastcheck — six core GPU lanes run automatically via Buildkite (~10-15 min).
- Merge gate — a reviewer adds
ready; changed paths select only the relevant integration, training, golden, or SSIM coverage.
Before your PR is reviewed:
-
pre-commit run --all-filespasses locally - You've added or updated tests for your changes
- The PR description explains what and why
If pre-commit fails, a bot comment will explain how to fix it. Fastcheck and merge-gate results appear in the Checks section below.
Useful links:
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
21913ff to
942a5c6
Compare
|
@fv-superagent review |
[Medium impact] The changes reach multiprocessing MPS inference and Studio’s output workflow. No behavioral blocker was found, but the comment-rule violation must be fixed before merging. Findings
Fix with agent prompt### Issue 1
apps/fastvideo_studio/tests/test_inference_output.py:66
This comment repeats the `_prepare_output_path` call below it. The repository requires removing comments outside its keep-list, which allows license headers, public API docs, required dependency or protocol explanations, formatting directives, and issue links-not narration.
The test module’s opening docstring and `_copy_mps_tensors_to_cpu`’s docstring have the same problem, with similar narration elsewhere in the added tests. Remove these comments before merging to satisfy the repository requirement.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
|
fv-superagent score: 4/5 The approach is right: PyTorch has no MPS storage-sharing path over the multiprocessing pipe ( Two non-blocking notes:
Greptile's single P2 (a redundant test comment) is fixed on the branch in f179370. — fv-superagent · run by @aryan5v |
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 3/5 (was 4/5)
The MPS half is unchanged and still looks right. Reviewing the new Studio commit 9b7c2b92f turned up one must-fix (the same issue Greptile flagged).
Must fix:
- Image jobs named with an image extension now produce a
.jpg/.jpeg/.webpfile that Studio can't serve, preview, or download correctly.
apps/fastvideo_studio/job_runner.py:1108passes the rawsafe_nameintooutput_video_name, and for an image workload_prepare_output_pathhonours an image extension in that name (fastvideo/entrypoints/video_generator.py:764-770), so a job namedportrait.jpgwritesportrait.jpg. Butapps/fastvideo_studio/server.py:664labels every non-.mp4outputimage/png,apps/fastvideo_studio/src/app/gallery/page.tsx:12treats only.pngas an image, andapps/fastvideo_studio/src/components/jobs/JobCard.tsx:152downloads everything that is not.pngas.mp4. Job names are unconstrained (CreateJobRequest.name), so this is reachable from the UI and API.
Either keep Studio image outputs as PNG (don't forward an image extension throughoutput_video_name), or extend the media type, gallery preview, and download-extension logic to cover.jpg/.jpeg/.webp. A regression that names a t2i jobportrait.jpgwould pin whichever you pick.
Non-blocking, carried over from the earlier review:
-
execute_methodresponses —execute_streaming_reset/execute_streaming_step(fastvideo/worker/multiproc_executor.py:245-267, dispatched through the else arm at :882-887) andstreaming_queue_loop(:929-936) still send device tensors raw, so MPS streaming would hit_share_filename_too. Out of scope for #1846. -
The MPS conversion path has no CI coverage: every
("mps", …)case skips without Apple hardware, and the("cpu", "all")case bypasses the helper.
— fv-superagent, automated maintainer run by @aryan5v
There was a problem hiding this comment.
fv-superagent score: 4/5 (was 3/5)
The P1 is fixed and now covered: apps/fastvideo_studio/job_runner.py:1104-1106 forces .png for 2i workloads before handing the name to the generator, so portrait.jpg / .jpeg / .webp become portrait.png, and the test asserts both the saved artifact and the served bytes are PNG (image.format == "PNG"). Job.workload_type is a str, so the endswith("2i") gate is safe there.
Thanks also for test_response_preparation_rebuilds_nested_containers_on_cpu — that is the Linux-runnable coverage for the dataclass / namedtuple / OrderedDict / PipelineLoggingInfo walk, and it pins type reconstruction and source immutability, which is what the MPS-only cases could not do in CI.
The only remaining item is the earlier non-blocking scope note: execute_method responses — execute_streaming_reset / execute_streaming_step (fastvideo/worker/multiproc_executor.py:245-267, via the else arm at :882-887) and streaming_queue_loop (:929-936) — still send device tensors raw, so MPS streaming would hit _share_filename_. Fine as a follow-up for #1846.
CI update — the Unit lane hangs after the tests finish, so fastcheck cannot go green on this head. buildkite/pr-fastcheck/microscope-unit-tests completes the whole session (3891 passed, 75 skipped, 5 deselected in 454s) and then the step never exits: it sat until the 90-minute step timeout on build #2252 (exit 124), and the same happened on 9b7c2b92f (build #2251: pytest finished in 450s, step still running 10+ min later when the next push cancelled it). Normal Unit runs finish in ~8 min (#2232–#2234), so this is a post-run teardown hang, not a failing assertion. pre-commit and build are green.
Can you reproduce and bisect locally? The lane's explicit list gains apps/fastvideo_studio/tests/test_inference_output.py plus the new worker tests:
pytest apps/fastvideo_studio/tests/test_inference_output.py fastvideo/tests/worker/test_multiproc_executor.py -q -p no:cacheprovider
echo $? # does the shell return after the summary, or hang?If it hangs on exit, something live is not being reaped — the Studio tests bring up JobRunner (spawn mp.Manager in job_runner.py:347, QueueListener) through a TestClient, and the worker tests spawn MultiprocExecutor children. py-spy dump or faulthandler.dump_traceback_later on the stuck process will name it. If it does not reproduce locally, say so and we will treat it as a Slurm step-teardown issue and re-run the lane.
— fv-superagent · run by @aryan5v
There was a problem hiding this comment.
fv-superagent score: 3/5 (was 4/5)
Correction after the Unit lane finished: the media-type inference does not hold for .webp here. apps/fastvideo_studio/tests/test_inference_output.py::test_named_job_serves_generated_artifact[t2i-portrait.webp-portrait.webp-image/webp] fails with assert 'text/plain; charset=utf-8' == 'image/webp' — FileResponse falls back to mimetypes.guess_type, and CPython's built-in map has no .webp entry (it only resolves via the host's /etc/mime.types, which this CI image lacks). Fix: mimetypes.add_type("image/webp", ".webp") at import, or an explicit media-type map in get_video. Details in my comment below.
The rest of the rework is the right direction: portrait.jpg/.jpeg keep their name, imageio.imwrite writes the encoding matching the extension, isImage covers all four image extensions, and the download keeps the real basename. The png/jpg/jpeg/mp4 cases pass; only .webp fails.
Also still open: the Unit lane's post-run teardown hang — pytest finished (1 failed, 3890 passed in 455s) and the step then sat until the 90-minute timeout (exit 124, build #2279), same as builds #2252/#2251. Repro command below.
pytest apps/fastvideo_studio/tests/test_inference_output.py fastvideo/tests/worker/test_multiproc_executor.py -q -p no:cacheprovider
echo $?Carried-forward scope note, no action needed here: execute_method responses (execute_streaming_reset/step) and streaming_queue_loop still send device tensors raw, so MPS streaming would hit _share_filename_ — a follow-up for #1846.
— fv-superagent · run by @aryan5v
|
Unit lane on this head has a real failure, not just the timeout.
Cause: mimetypes.add_type("image/webp", ".webp")or pass an explicit Separately, the Unit step still hangs after pytest finishes (the run above took 455s, then — fv-superagent · run by @aryan5v |
There was a problem hiding this comment.
fv-superagent score: 5/5 (was 4/5)
Confirmed on CI: all six fastcheck lanes, fastcheck-passed, pre-commit and build are green on 14773cb, so the post-run teardown hang is gone. Both fixes hold in the run — .webp serves image/webp, and the in-process transport test no longer shuts down the shared Inductor pool.
For the record, the PR: MPS worker responses are copied to CPU before pipe.send; Studio artifact names keep their real format end to end (png/jpg/jpeg/webp), with serve/download and transport isolation covered by tests. The only carried-forward item is the out-of-scope execute_method/streaming MPS transport, a follow-up for #1846.
Nothing blocks. Greptile's review predates the last two heads; its P1 (image names break downloads) is fixed and covered by the new tests.
— fv-superagent · the compiler pool lives to fight another test · run by aryan5v
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 5/5 (was 5/5)
Test-only delta, and a good one: the spawn round-trip test now covers cpu, mps and cuda. The expectations are right — _run_output_worker stubs current_platform.is_mps per case, so CPU and CUDA tensors cross the pipe as-is (the CUDA case asserts cuda) while the MPS case still returns on CPU through _copy_mps_tensors_to_cpu. The assert_tensor helper plus the explicit response = output = metadata = metric = None releases keep imported CUDA/CPU storage from outliving the producer process, which is the correct discipline for IPC-backed tensors. No gpu marker, so the CUDA case runs in the fastcheck Unit lane (which has CUDA) and skips where it does not.
This sits on top of the 5/5 on the previous head, where all six fastcheck lanes, fastcheck-passed, build and pre-commit were green. CI is re-running on this head; nothing blocks.
— fv-superagent · three devices, one pipe, no leaks · run by aryan5v
|
Update: the native PyTorch/MPS video check through Studio is now complete, together with the image check, on rebased head @fv-superagent review Could you review head python -m pytest -vv -rs \
'fastvideo/tests/worker/test_multiproc_executor.py::test_execute_forward_response_survives_spawn_and_repeated_requests[cuda]' \
'fastvideo/tests/worker/test_multiproc_executor.py::test_response_preparation_preserves_non_mps_tensor_identity[cuda]' \
fastvideo/tests/worker/test_multiproc_executor.py::test_prepare_worker_output_moves_latent_cuda_tensor_to_cpu
echo "pytest_exit=$?"This needs one GPU and tiny tensors, with no model download. The new case checks real CUDA IPC through the spawned worker/pipe, CUDA device retention, exact nested values, two requests, and release of received storage before producer shutdown. Local verification on the same SHA is complete:
After the requested five-minute wait, I found no unresolved review threads; your latest review was positive and the new CI run was still pending. Please send the CUDA execution result back here so we can distinguish source review from GPU runtime validation. |
|
Verified on the Linux NVIDIA runner — all three CUDA cases ran and passed; nothing was skipped.
Pytest exited normally: job state For completeness, the existing CUDA cases also passed on the previous head's run (build #2282, Review of — fv-superagent · CUDA passed, MPS stayed home · run by aryan5v |
6612850 to
8737f1b
Compare
fv-superagent
left a comment
There was a problem hiding this comment.
fv-superagent score: 5/5 (was 5/5)
The branch was rewritten since my last review (head 6612850 → 8737f1b): the MPS change is now four commits and the tree is rebased. I re-diffed the production files against main and the net change is identical to what I reviewed — fastvideo/worker/multiproc_executor.py still adds _copy_mps_tensors_to_cpu plus the current_platform.is_mps() branch in worker_busy_loop, and the Studio changes (job_runner output name/returned path, server mimetypes.guess_type(..., strict=False), the gallery/JobCard extension handling) are unchanged. No test coverage was dropped: test_execute_forward_response_survives_spawn_and_repeated_requests is still parametrized over cpu/mps/cuda.
The new commit is a good one: apps/fastvideo_studio/tests/test_duplicate_and_edit.py now yields the JobRunner and calls _shutdown() in finally, so each test releases its mp.Manager instead of leaving a live child for interpreter exit — the same hygiene as test_inference_output.py.
My CUDA verification still holds: the CUDA case content is unchanged (it is now cfc07408b; test_execute_forward_response_survives_spawn_and_repeated_requests[cuda], test_response_preparation_preserves_non_mps_tensor_identity[cuda] and test_prepare_worker_output_moves_latent_cuda_tensor_to_cpu all passed on the GB10 runner). CI is re-running on this head.
— fv-superagent · the history got a rewrite, the diff didn't · run by aryan5v
|
@fv-superagent review Thanks for confirming the CUDA passes and reviewing the rebase. The local functional checks are now complete, and this is ready for final review at On the M5 Pro, both examples passed through the real Studio API, JobRunner and explicit native PyTorch/MPS
The Wan diagnostic needed the existing After the rebase: 93 passed, 4 skipped locally, exit 0; all pre-commit hooks passed. The only extra commit closes Studio test fixture managers. The production diff and CUDA test content are unchanged. Build #2289 now has green Unit, Kernel and Dreamverse app checks; the remaining Fastcheck work is still pending. If the Unit log already records the three CUDA cases on this new SHA, please link that confirmation—no duplicate run is needed. Could you give the final review and flag any remaining maintainer approval or merge-gate requirements? The PR description now follows the template and contains the updated Mac/CUDA evidence. |
|
Final review is in, and the CUDA cases are confirmed on this SHA.
Remaining merge-gate requirements (Mergify):
So nothing further is needed from my side; the approval and merge are the maintainers' call. Your M5 Studio/MPS image+video report is noted, and using the existing documented — fv-superagent · the rebase didn't shake CUDA loose · run by aryan5v |
|
@fv-superagent review Automatic CI is now green on The local native MPS image and video functional checks also passed on this SHA, as documented above. The PR's Purpose section now explains both concrete failures and includes the named-image path/download before-and-after example. Could you give the final review and confirm the remaining maintainer approval/merge-gate steps? A new Greptile comment about redundant comments/docstrings arrived as CI finished; please include that in the final review. |
Purpose
Fixes #1846 and a separate named-image download bug found while verifying the fix in Studio.
1. MPS generation finishes, but the worker cannot return the result.
When Studio uses
MultiprocExecutoron a Mac, the worker can finish FLUX generation successfully and then fail atpipe.send(response):The response can contain MPS tensors in both the generated output and nested metadata/logging. PyTorch 2.12.0 cannot serialize their MPS storage through this multiprocessing pipe. The parent therefore never receives a usable result, even though GPU computation finished. This can happen between two Python processes on the same Mac; it transfers the result, not the model.
The fix copies MPS tensor leaves to CPU immediately before sending the response, including nested tensors. CPU/CUDA responses keep their existing path.
2. A named image job says “completed,” but its download returns 404.
Studio previously appended
.mp4to every job name and passed that full path asoutput_path, including for image jobs. For an image workload, the generator treats that mismatched path as a directory and saves the PNG inside it. Studio then searches for*.mp4, mistakenly selects that directory, and records it as the generated file.For a fresh image job named
portraitwith prompta red dinosaur:outputs/<job>/portrait.mp4/a red dinosaur.pngoutputs/<job>/portrait.pngoutputs/<job>/portrait.mp4— a directoryThis second bug is platform-independent. The fix passes the output directory and optional filename separately, records the generator's returned path, and requires it to be a real file. Supported image names such as
portrait.jpgandportrait.webpkeep their formats; serving, preview and download code now recognize them correctly.PyTorch has different ways of doing this for different devices:
The relevant PyTorch code is the CUDA tensor branch, storage reducer, and CPU-storage check that raises the error.
Changes
execute_forwardresponse. Other platforms send it directly.tensor.detach().cpu(), including tensors nested in dictionaries, lists, tuples, named tuples, dataclasses, and stage logging.output_video_nameand record the generator's returnedvideo_pathinstead of guessing from directory entries. Require the returned path to be an actual file.strict=False) determine the content type from the saved path, including WebP without system MIME files; the gallery recognizes those image formats, and downloads retain the actual saved filename and suffix..jpg,.jpeg, and.webp, verify decoded image formats and HTTP content types, and ignore stale files/directories. Include the backend regression file in the existing unit-test CI lane. Frontend tests cover image previews and download names, including a Windows server path./etc/mime.types.The MPS conversion applies only to the
execute_forwardresponse inworker_busy_loop. Genericexecute_methodresponses, streaming reset/step calls, and streaming queues are unchanged; MPS streaming support is outside this PR. Executor selection is unchanged. MPS copies preserve values, shape, and dtype; they do not carry autograd history or preserve shared storage between distinct views.Current single-device behavior: #1879 already selects
UniprocExecutorfor the defaultnum_gpus=1configuration. The full-model diagnostic explicitly selected the existingMultiprocExecutorto exercise the failing handoff.Test Plan
Rebased onto main
866a3cdb91a85aef8561847365fc8564add9e4c8; current head is8737f1b640db7181c551654d377c817fe83f4a33. The rebase preserved the production diff and existing test coverage. One additional fixture cleanup closes Studio test managers after use.Local hardware/software: Apple M5 Pro, 48 GB unified memory, macOS 26.6.2, Python 3.12.13, PyTorch 2.12.0.
Full-model diagnostics use FastAPI TestClient with the real Studio handlers, database, JobRunner, model pipeline, saved file, and media response. They explicitly select the existing
MultiprocExecutorto exercise this PR's handoff. These are API/backend checks, not browser automation.black-forest-labs/FLUX.2-klein-4B, revisione7b7dc27f91deacad38e78976d1f2b499d76a294; prompta red dinosaur; 832 x 480, 1 frame, 4 steps, guidance 1, seed 1024; nameRebased Mac image.webp.Wan-AI/Wan2.1-T2V-1.3B-Diffusers, revision0fad780a534b6463e45facd96134c9f345acfa5b; orange-cat walking prompt; 50 steps, guidance 5, seed 1024, 16 fps; dimensions/frame count below.The native Wan diagnostic sets the existing public configuration
pipeline_config.text_encoder_precisions = ("bf16",)to match its BF16 transformer on MPS. Stock FP32 text conditioning aborts in a Metal dtype mismatch before the result handoff. Studio's form does not expose this override: this validates a configured native MPS backend path, not out-of-box Wan support through the form. No Wan model, precision-default, or executor-selection changes are included in this PR.Test Results
_share_filename_: only available on CPU.8737f1b648737f1b64image/webp; visual check and clean exit pass.8737f1b64video/mp4, valid H.264 at 256 x 256, 25 frames, 16 fps (1.5625 s); full-file decode and clean process exit passed.8737f1b648737f1b64fastcheck-passed=success; pre-commit also passed.text/plain. After: 6 passed with PNG/JPEG/WebP/MP4 content types.8737f1b64The native video is a deliberately small functional smoke: its output is blurry, and image quality is not an acceptance criterion for this result-transfer/download fix. The generated MP4 has all requested frames and decodes completely. The BF16 text-encoder configuration required for this Mac run is disclosed above. A separate optional 480p quality check was stopped when we agreed to keep validation focused on the interface; it is not counted as a passing test.
Test output and validation boundaries
The CUDA round trip uses real CUDA tensors and a spawned pipe, with a fake inference worker; it is not full-model CUDA generation. The linked runner report confirms all three CUDA cases executed on the current rebased SHA. The production diff and test content were preserved by the rebase, as independently confirmed in the follow-up review.
Build #2279 had a WebP assertion failure and a separate post-summary timeout. Later builds #2282 and #2288 exited normally; the rebased Unit lane is also green. The test cleanup fixes reproduced local process leaks, but it does not establish the root cause of the earlier Slurm timeout.
Earlier broader worker tests had four distributed-rendezvous failures with hostname-resolution errors on this Mac; the same four reproduce on unchanged upstream. CUDA runtime evidence comes from the NVIDIA runner, not this Mac. No performance improvement or cross-device output-quality parity is claimed.
Frontend tests used a temporary checkout without spaces because the existing Vitest alias configuration URL-encodes the workspace path. The all-files hooks used the docs lockfile dependencies.
Checklist
pre-commit run --all-filesand fixed all issuesModel/pipeline and new-model/serving-runtime checks: Not applicable. This PR changes result transport, adds no model/runtime, and does not change model calculations, serving YAML, or the support matrix.