Skip to content

feat(minimax-h3): integrate native video-audio apps with V2 - #523

Draft
jmccaffrey-nv wants to merge 3 commits into
dev/jmccaffrey/v2-audio-outputfrom
dev/jmccaffrey/minimax-h3-integ-v2
Draft

feat(minimax-h3): integrate native video-audio apps with V2#523
jmccaffrey-nv wants to merge 3 commits into
dev/jmccaffrey/v2-audio-outputfrom
dev/jmccaffrey/minimax-h3-integ-v2

Conversation

@jmccaffrey-nv

@jmccaffrey-nv jmccaffrey-nv commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #443.

Depends on #526, which contains the model-neutral V2 synchronized-audio and transactional MP4 runtime. This draft is temporarily based on the exact #526 branch head so the visible diff is H3-only.

  • Port MiniMax H3 from Add MiniMax H3 integration #457 onto the V2 application/runtime design introduced by Document the v2 API, runtime and integrations #515.
  • Run T2VA, FL2VA, and REF2VA through native FlashDreams inference. Production code does not import Diffusers, PyAV, a Python/bundled FFmpeg, or a V1 runner.
  • Add native transformer, video VAE, audio VAE, text/keyframe/reference conditioning, checkpointing, and staged device placement.
  • Decode local video and audio references through external host ffmpeg and ffprobe executables behind a core input boundary.
  • Pin and enforce the MiniMax model snapshot revision and retain native/oracle provenance.
  • Register minimax-h3-t2va, minimax-h3-fl2va, and minimax-h3-ref2va as V2 applications.

Split history

The original combined head 81c68f4 passed all local and GitHub CPU/GPU checks. It is preserved at branch dev/jmccaffrey/minimax-h3-integ-v2-pre-split-81c68f46. Generic runtime/audio review fixes moved to #526; H3 and media-input review fixes remain here. The rebuilt combined tree is identical except that one unrelated legacy runner_io write-video -nostdin line was omitted. No H3 math, weights, scheduling, or device-placement behavior changed.

Validation

Split H3 head: 3cfdd23.

  • Full pre-commit gate: passed, including Ruff check/format, uv lock validation, package-version sync, and full-workspace ty.
  • Exact repository CPU tier: 2,100 passed, 2 skipped, 346 deselected in 87.51 seconds.
  • Focused native H3, V2 adapter, and host-media suite: 138 passed, 3 deselected.
  • FlashDreams core, native H3, and H3 V2 sdists/wheels: all built successfully.
  • Native CUDA parity at the immutable artifact-validation head: 3 passed, 82 deselected.
  • DCO: 3 / 3 H3 commits signed off; feat(runtime-v2): add synchronized audio output #526 is 2 / 2.

The eight Apache-2.0 native components modified from the pinned Hugging Face Diffusers revision retain their applicable upstream copyright inline and are enumerated in both REUSE.toml and THIRD-PARTY-NOTICES. Model weights are downloaded from the pinned upstream revision and are not redistributed. Host FFmpeg/FFprobe executables are invoked externally and are not bundled or linked.

Real-weight RTX PRO results

Eight real-weight MP4s were generated on an NVIDIA RTX PRO 6000 Blackwell Workstation Edition. Every artifact decoded end-to-end through both streams and probed as H.264 High/yuv420p at 24 fps plus AAC-LC 32 kHz stereo. Exact commands, hashes, probes, environment, codec evidence, and the complete gallery record are in integrations_v2/minimax_h3/ROLLOUT.md.

Run Request Model total Wall Generated fps Peak GPU
T2VA canonical median 768x768, 5 s, 30 grid points / 29 updates 223.094 s 238.46 s 0.5558 65.569 GiB
FL2VA 768x768, 5 s, 30 grid points / 29 updates 255.376 s 271.38 s 0.4856 65.956 GiB
REF2VA video + audio 768x768, 5 s, 30 grid points / 29 updates 626.933 s 643.43 s 0.1978 69.871 GiB
15-second mux boundary 256x256, 15 s, 2 grid points / 1 update 49.971 s 84.15 s 7.2442 63.267 GiB

The 15-second run produces 362 frames and aligns the padded AAC stream within 0.667 ms of the written-video duration. Real-weight results remain tied to artifact-validation head 4e91a25, preserved through the archive branch above.

Notes

  • Model revision: 42ed227ee7df40d41602854ae760620d6eb651fe.
  • Diffusers parity oracle: 175fe6b2419a01db9c2ceabd01ec37d2c0305fc2.
  • --steps N counts sigma-grid points including terminal zero, so N performs N-1 Euler/model updates.

@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 9744c5f

@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 227491a

@jmccaffrey-nv
jmccaffrey-nv marked this pull request as ready for review August 26, 2026 14:45
@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 85308e3

@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR ports MiniMax H3 text-, first-frame-, and reference-conditioned video-audio generation onto the V2 application/runtime contract.

  • Adds native MiniMax H3 model components, inference flows, package registrations, and model-specific tests.
  • Extends V2 session results and presentation with synchronized mono or stereo audio.
  • Adds transactional MP4 staging, external FFmpeg AAC muxing, atomic publication, and failure cleanup.
  • Adds host-media decoding helpers, dependency metadata, licensing records, and rollout documentation.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains in the available follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
flashdreams/flashdreams/runtime_v2/session_runner.py Coordinates session execution with the revised output commit and abort lifecycle.
flashdreams/flashdreams/runtime_v2/mp4_output_sink.py Implements private staging, transactional publication, and cleanup ownership for MP4 output.
flashdreams/flashdreams/runtime_v2/mp4_audio.py Adds normalized PCM staging, AAC capability preflight, timeline alignment, and external FFmpeg muxing.
flashdreams/flashdreams/runtime_v2/audio_output.py Defines and validates the V2 synchronized-audio result contract.
flashdreams/flashdreams/core/io/media_input.py Adds model-neutral host FFmpeg and FFprobe decoding for video and audio inputs.
integrations/minimax_h3/minimax_h3/inference.py Implements the native MiniMax H3 inference orchestration used by the integration.
integrations_v2/minimax_h3/minimax_h3_v2/app.py Adapts MiniMax H3 generation modes to V2 application and session contracts.
integrations_v2/minimax_h3/pyproject.toml Packages and registers the three MiniMax H3 V2 application slugs.
uv.lock Records the workspace dependency closure for the added integration packages.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[MiniMax H3 V2 application] --> B[Native model inference]
    B --> C[Video frames and synchronized PCM]
    C --> D[V2 session and presentation]
    D --> E[Transactional MP4 sink]
    E --> F[Stage H.264 video]
    E --> G[Stage interleaved PCM]
    F --> H[External FFmpeg mux]
    G --> H
    H --> I[Atomic MP4 publication]
Loading

Reviews (2): Last reviewed commit: "docs(minimax-h3): record review validati..." | Re-trigger Greptile

@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

5 second t2v outputs :

10-rooftop-robot-jazz.mp4
12-lunar-greenhouse.mp4
08-storm-lighthouse.mp4

@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

In-depth review: H3 model integration

Reviewed at 85308e3c against origin/main (e6e1c002) — 66 files, +14,895/−76. Read in full: the flashdreams/ runtime and core/io changes, integrations_v2/minimax_h3, and the NVIDIA-authored native modules (inference.py, model.py, latent_checkpoint.py, constants.py, scheduler.py, transformer.py scaffolding); skimmed the ported Diffusers components (video_vae.py, audio_vae.py, conditioning.py, reference_conditioning.py, text_encoder.py) and the test suite.

Overall: the V2-side design is right. The audio contract is model-neutral and lands where it belongs (StepResult.audio + SessionDesc.audio_* + AbortableOutputSink), the MP4 transaction is genuinely transactional, host FFmpeg stays an external executable, and the docs (api_v2/README.md, runtime_v2/README.md, integrations_v2/README.md) were updated alongside the contracts they describe. The native port leans on core.attention.NativeAttention / apply_rope_freqs, core.checkpoint.load, and infra.diffusion.{model,scheduler,transformer} rather than re-inventing them, and test_rope_kernel.py::test_partial_prefix_view_matches_torch pins exactly the sliced-view in-place contract transformer._apply_rotary depends on — that's the right instinct.

The findings below are ordered by severity. Sections 3 and 4 answer the SLoC and reuse questions directly.


1. Correctness

1.1 — BaseException.add_note() breaks the declared Python 3.10 floor. (blocking)

video_encoder.py:125,138,184,224 and mp4_output_sink.py:108,115,121,241,254 call cast(Any, error).add_note(...). add_note is 3.11+; flashdreams/pyproject.toml:26 and every integration manifest declare requires-python = ">=3.10". All nine sites sit in cleanup paths that are already handling an in-flight exception, so on 3.10 the resulting AttributeError doesn't just lose a note — it replaces the original failure.

The repo already has the 3.10-safe idiom for this, twice: demo/bridge.py:795 (_add_exception_note) and demo/application.py:476. The cast(Any, ...) is a hint that the type checker was working around the same thing. Promote _add_exception_note to flashdreams/core and call it here.

1.2 — FlashVSR silently gains an ffprobe runtime dependency.

infra/runner_io.py:163-170 reroutes read_video_fps to host FFprobe and does del install_hint # Kept for API compatibility. integrations/flashvsr/flashvsr/runner.py:176 calls it — and that same runner still reads pixels through read_video_rgb, which is unchanged mediapy. So FlashVSR now needs both mediapy and ffprobe on PATH, where before it needed only mediapy, and the install_hint the caller passes is discarded rather than surfaced when the new dependency is missing.

This is blast radius outside the H3 scope. Either keep a mediapy fallback when ffprobe is absent, or make it an explicit, called-out dependency change with the hint text updated to say what to install.

1.3 — avg_frame_rate of 0/0 is rejected instead of falling back to r_frame_rate.

core/io/media_input.py:166:

rate = stream.get("avg_frame_rate") or stream["r_frame_rate"]
fps = float(Fraction(rate))

"0/0" is a truthy string, so the or never fires. Fraction("0/0") raises ZeroDivisionError, which :168 catches and re-raises as ffprobe returned invalid video metadata for {path}. FFprobe emits avg_frame_rate=0/0 routinely for VFR, fragmented, and stream-copied inputs whose r_frame_rate is perfectly valid. --reference video:<such a file> therefore fails at argument-decode time with a message that blames the file.

Try each candidate in turn and take the first that parses to a positive finite rate.

1.4 — read_video_rgb_with_fps holds the decoded clip twice.

media_input.py:69 uses subprocess.run(..., capture_output=True), which buffers the entire raw RGB stream into process.stdout, and :81-82 then does np.frombuffer(...).reshape(...) followed by .copy(). Peak is 2× the raw decoded size. A 1080p/30 s reference video is ~5.6 GB raw, so ~11 GB resident — in the process that is about to load an H3 transformer. runner_io.read_video_rgb (mediapy) did not have this shape.

Probe the frame count during _probe_video_stream and stream the pipe into one preallocated array, or decode in chunks. At minimum the .copy() can go by reading into a preallocated buffer.

1.5 — Mp4AudioMuxer.close() has no timeout.

mp4_audio.py:252 calls process.communicate() unbounded, while preflight_audio_codec bounds its one-frame probe at 10 s (:82). A wedged mux hangs the run permanently, and the "retry interrupted cleanup" path can't help: abort() runs on the same thread that communicate() is blocking, so self._process is only ever reachable if communicate() raised. Give the mux a timeout scaled to the clip and kill on expiry.

1.6 — The runtime may call IApplication.close() twice; the protocol doesn't say so.

application_runner.py:60-64 sets application_closed = True only after self._application.close() returns. If it raises, run_session catches it into cleanup_failures and the outer finally at :80 sees application_closed is False and closes again. test_application_runner.py::test_application_close_failure_aborts_transactional_output asserts calls.count("application.close") == 2, so this is deliberate — but api_v2/application.py:60-66 documents close as "Release whatever the application holds" with no idempotence requirement, and api_v2/README.md wasn't updated. MiniMaxH3Application.close happens to guard on self._closed; a third-party application following the documented contract has no reason to.

Either document it on the protocol and in the README, or set the flag in a finally so the double call doesn't happen.

1.7 — Partial commit is possible across multiple abortable sinks.

In session_runner's abortable loop, sinks are committed in list order; when sink n fails to close, abort_transactions flips to True and sinks n+1… are aborted — but sinks 0…n-1 have already published. With a single MP4 sink this cannot bite today. It can as soon as a second abortable sink exists (an abortable metrics sink is the obvious one). Worth either a comment stating the limitation or a two-phase prepare/commit split.


2. Efficiency and smaller notes

  • normalized_pcm() re-validates an already-validated frozen dataclass. audio_output.py:87 calls the full _validate_samples that AudioOutput.__post_init__ already ran — two torch.isfinite(...).all() passes and two range scans per payload, each a device sync when the payload is still on GPU. Validate once at construction; normalized_pcm only needs the dtype/device/contiguity conversion.

  • File-output runs pay one full real-time playback of the clip. app.py:204 sets frames_per_second_for_ui = FPS = 24, and session_runner.py:78 derives tick_seconds = 1/fps_ui, gating each UI tick on stop.wait(tick_seconds). Under ONLY_PRESENT_NEW that's one frame written per tick, so writing the MP4 costs num_frames / 24 seconds of pure waiting after generation finishes. Your own table shows it: the 15-second boundary run is 49.97 s model total vs 84.15 s wall. frames_per_second_for_ui does not affect the output file (the encoder uses frames_per_second_for_step), so raising it for the MP4 path is free wall-clock.

  • Per-step checkpointing copies each stream four times plus a SHA-256. model.py:424 constructs a MiniMaxH3DenoiseProgress per denoise step; its __post_init__ (model.py:122-131) runs torch.isfinite(...).all() on both packed streams (device sync), the constructor already did .detach().clone(), then latent_checkpoint._checkpoint_tensor:78 does .to("cpu").contiguous().clone() and _tensor_sha256:82 does another .contiguous().view(uint8).numpy().tobytes() plus a full hash — for a checkpoint most runs never resume. Consider hashing the safetensors payload once rather than each tensor, and dropping the finite check from the per-step construction path.

  • Mp4OutputSink._clear_transaction(*, clear_staging: bool = True) (:288) — never passed by any of its three call sites (:179, :265, :286). Dead parameter.

  • --steps 30 runs 29 Euler updates. scheduler.py:58-64 builds linspace(1, 0, num_inference_steps) and takes timesteps = 1 - sigmas[:-1]; latent_checkpoint.py:205 encodes the same as update_count = num_inference_steps - 1. Internally consistent and evidently matching the oracle, but it differs from the Diffusers convention a user reading --steps will assume — one line in the README would settle it. Separately, torch.unique_consecutive at :62 makes the update count silently data-dependent if the shifted grid ever collides in float32.

  • SessionDesc fps validation tightened from math.isfinite to isinstance(int). Every in-tree caller is already int (cli.py:144, t2v_v2/application.py:147-148, red_screen/app.py:202), so nothing here breaks — but api_v2 is a published surface and this is a hard break for any out-of-tree V2 application passing 29.97. Worth a line in the api_v2 README or release notes.

  • A dropped chunk becomes silence with no signal. Because sample_offset is absolute, a chunk dropped under BackpressureMode.DROP_OLDEST has its audio quietly replaced by padding in F32leAudioStager. H3 requires BLOCK, so it can't hit this — but api_v2/README.md describes gaps as "explicit silence" without saying that dropping is one of the ways to produce one.


3. Can the SLoC be reduced?

Where the 14,895 lines actually go:

Area Added Share
Native H3 model source (integrations/minimax_h3/minimax_h3) 6,996 47%
Tests (all packages) 5,548 37%
FlashDreams runtime + core/io 1,232 8%
V2 application (integrations_v2/minimax_h3) 537 4%
Docs, manifests, uv.lock 582 4%

First, fair context: 7,000 lines of native model source is in band for this repo. flashvsr is 9,979, sana 9,786, lingbot 7,862. Two VAEs, a 50-layer joint transformer, and Qwen3-VL conditioning is simply that much code, and it is not where the reducible mass is. The reducible mass is in four places:

(a) The validation layer — realistically 600-900 lines. 595 lines of the native package sit in dedicated __post_init__ / _validate_* helpers, holding 127 raise sites, with another 172 raise sites inline across ordinary functions. Much of it re-checks internal, already-typed, single-caller data at every layer: MiniMaxH3DenoiseState is constructed by conditioning.prepare_denoise_state, then re-validated field-by-field by model._validate_state (model.py:150-268, 118 lines) including a full index-partition check. text_encoder.py:57-59 iterates every token id in Python on a value the tokenizer just produced. inference.py alone is 125 lines of validators.

Keep the boundary checks — MiniMaxH3InferenceRequest, AudioOutput, SessionDesc, checkpoint load — and drop the interior ones and you lose no guarantee a caller can actually observe. What remains collapses further with table-driven helpers: _positive_int(name, value) already exists as _strict_positive_integer at latent_checkpoint.py:44 and is used in exactly one file. A _finite_float_tensor(name, tensor) would take out most of the rest.

(b) The repeated cleanup idiom — ~60 lines → ~10. if failure is None: failure = error else: cast(Any, failure).add_note(...) appears nine times across video_encoder.py and mp4_output_sink.py. One helper — _record(failure, error, note) -> BaseException — collapses all of them, removes nine cast(Any, ...) type-checker escapes, and fixes finding 1.1 in the same edit.

(c) Duplicated test doubles — ~180 lines, and two copies already drifting. _Tokenizer, _ImageProcessor, _VideoProcessor, _Processor, _QwenBase, _TextEncoder are each defined twice with different behavior: tests/test_text_encoder.py:41-140 uses a frozen lookup table, tests/test_inference.py:53-135 uses 12 + len(text) % 100. Neither H3 test package has a conftest.py. flashdreams/t2v_v2/testing.py is the existing precedent for a shipped testing module; a minimax_h3/testing.py or shared fixtures would collapse these and stop the divergence. The 63 pytest.raises assertions in the H3 tests mostly exist to cover the validation in (a) and shrink with it.

(d) ROLLOUT.md — 215 lines with no precedent in the repo. It's the only file of its kind in the tree, and it's run evidence for one rollout on one specific GPU, which is stale the moment the head moves (the doc itself notes full-quality validation ran at 0b3ed9e8, not at head). Most of it is already in the PR description, which is the right home for it. Keeping the reproducible commands in the integration README and letting the measured numbers live in the PR would drop this to ~40 lines.

Realistic total: roughly 1,000-1,400 lines, most of it in tests and validation, without removing a single capability or a single meaningful guard.


4. Could it lean harder on FlashDreams common components?

Already done well, and worth saying: core.attention.NativeAttention and apply_rope_freqs in transformer.py:28, core.checkpoint.load.load_checkpoint, infra.diffusion.transformer.Transformer / TransformerConfig / TransformerAutoregressiveCache, infra.diffusion.scheduler.{Scheduler,SchedulerConfig,FlowPredictor} in scheduler.py:26, infra.config.InstantiateConfig for both VAEs. That's the shape a native port should have.

The gaps:

4.1 — EventProfiler (infra/profiler.py:23) is exactly what inference.py hand-rolls. inference.py:470-533 defines a local synchronized_time() and threads eight _started/_finished pairs through generate() — about 55 lines — and calls torch.cuda.synchronize() nine times per generation, serializing the pipeline purely to time it. EventProfiler measures the same stages with CUDA events and one sync at the end (sync_and_summarize()), and record_event(profiler, stage) already handles the optional case. This is a near drop-in that removes both the code and the syncs.

4.2 — There is no single host-FFmpeg locator, and this PR makes that worse. shutil.which("ffmpeg") now appears in four places with four different error strings: core/io/media_input.py:262 and :273, runtime_v2/mp4_audio.py:52, runtime_v2/video_encoder.py:195, infra/runner_io.py:431 — and media_input._find_ffmpeg_binary is a near-verbatim copy of runner_io._find_ffmpeg_binary. A flashdreams.core.io.ffmpeg with find_ffmpeg(), find_ffprobe(), and a run_ffmpeg(args) that carries the common -hide_banner -nostdin -loglevel error prefix would consolidate all of it and give one place to make policy decisions. (video_encoder._command is the deliberate exception, since it reads frames from stdin.)

4.3 — _add_exception_note already exists at demo/bridge.py:795. See 1.1 and 3(b).

4.4 — Two DiagonalGaussianDistribution.sample() bodies, byte-identical. video_vae.py:127 and audio_vae.py:129 differ only in parameterization (chunked logvar vs separate mean/logs); the sampling body is the same seven lines. Small on its own, but FlashDreams has no diagonal-Gaussian helper and every ported VAE will keep bringing its own.

4.5 — Worth an explicit answer: should the H3 VAEs sit on infra/decoder/base.py? DecoderConfig / StreamingVideoDecoder is what flashvsr, omnidreams, cosmos_predict2, lingbot, and causal_forcing build on; MiniMaxH3VideoVAEConfig (video_vae.py:704) extends InstantiateConfig directly instead. Similarly, flashdreams/recipes/ (with its template/ showing config / encoder / decoder / transformer / runner) is where the wan and cosmos model families live, and integrations/wan21 is 461 lines because of it. If MiniMax H3 is a one-off, integration-local is right and this is a non-issue — but that decision isn't recorded anywhere in the PR, and it's the one that determines whether the next H3-family model costs 461 lines or 7,000. A sentence in integrations_v2/minimax_h3/README.md would settle it.

4.6 — _RecordingWindow is the eighth recording client window in the tree. test_app.py:121 joins test_session_runner.py:278, test_cli.py:146, test_client_window.py:25, test_application_runner.py:111, t2v_v2/testing.py:364, and red_screen's ScriptedClientWindow:35 — and this PR adds a ninth, TransactionalRecordingClientWindow (test_session_runner.py:335). One shared flashdreams.runtime_v2.testing (or a _pytest_plugins fixture) would serve all of them, and the abortable variant this PR needs would be a two-line subclass rather than a new file-local class.

4.7 — A OneShotApplication base is the reusable piece hiding in app.py. H3 is legitimately not a T2VApplication — it's finite, it has audio, it has ordered references. But app.py:279-350 re-implements --prompt / --seed / --device parsing and the init → create_session → close lifecycle that T2VApplication already factors behind _configure_argument_parser and _apply_parsed_arguments hooks (which is why t2v_wan21/app.py is 55 lines). With Waypoint and LingBot V2 apparently in flight, a flashdreams/oneshot_v2 sibling to t2v_v2 — shared arg parsing, a finite single-result ISession / IModelLoop, and the audio-declaring SessionDesc — would take roughly 200 of app.py's 517 lines into the framework and make the second and third such integration nearly free. That's the highest-leverage reuse item here, though it's reasonable to land H3 first and extract the base when the second consumer arrives — if that's the plan, say so in the README so it doesn't get copy-pasted instead.


Suggested order

  1. Fix 1.1 (add_note on 3.10) — it's a real break on a supported interpreter and it comes for free with the helper in 3(b).
  2. Decide 1.2 (FlashVSR / ffprobe) — it's the only change here that can break something outside this PR's scope.
  3. Fix 1.3 and 1.4 — both are in media_input.py, both affect REF2VA inputs, both are small.
  4. 1.5, 1.6, 1.7 and the section 2 items as follow-ups.
  5. Take 4.1 and 4.2 in this PR (mechanical, and 4.1 removes nine device syncs from the hot path). Record a decision for 4.5 and 4.7 in the README even if the refactor waits.

Nice work on the transactional MP4 path and on test_partial_prefix_view_matches_torch — that test is doing real work.

@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 81c68f4

@jmccaffrey-nv
jmccaffrey-nv marked this pull request as draft August 26, 2026 19:45
@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

Addressed the actionable findings from the Claude review, then re-reviewed the patch with three independent agents.

Fixed in this PR

  • 1.1 / 4.3: promoted a Python-3.10-safe add_exception_note helper and replaced all nine unsupported cleanup calls. Added supported/no-note compatibility tests.
  • 1.2: restored legacy infra.runner_io.read_video_fps to mediapy, including caller install-hint propagation, so FlashVSR does not acquire an ffprobe dependency. H3 continues to use model-neutral core host-media helpers directly.
  • 1.3: avg_frame_rate and r_frame_rate are now tried independently; only a positive finite rate is accepted. 0/0 fallback and both-invalid cases are tested.
  • 1.4: removed the duplicate full RGB NumPy copy. The decoder intentionally still buffers one complete clip in subprocess stdout; it now returns a documented read-only zero-copy view. A regression carries that view through H3 normalization and VAE encoding.
  • 1.5: final audio muxing now has a media-duration-scaled deadline, bounded terminate→kill waits, and ownership retention when a child cannot be reaped. Tests cover successful escalation, failed reap, later abort retry, and exact frames / fps wiring.
  • 1.6: application close is attempted once; a failing close is no longer retried against an undocumented idempotence requirement.
  • 1.7: documented that atomicity is per sink, not a cross-sink transaction.
  • Removed the dead _clear_transaction(clear_staging=...) option, documented dropped-audio silence, documented strict integer FPS, and clarified that --steps N is N sigma-grid points / N−1 updates.
  • Merged current main and preserved both Cam2V/LingBot presentation pacing and H3 synchronized audio/transaction behavior.

Intentionally retained or deferred

  • normalized_pcm revalidation stays: a frozen dataclass does not freeze the contained tensor, so sink-boundary validation still catches post-construction mutation.
  • EventProfiler is not a drop-in replacement for the current wall timings: it is CUDA-event-only and cannot preserve CPU, disk, component-load/release, or explicit-device timing semantics.
  • Checkpoint copies/hashes are opt-in (--work-dir/--job-id), not paid by ordinary runs; optimization can be benchmarked separately without weakening resume integrity.
  • MP4 real-time presentation pacing, shared FFmpeg locator policy, two-phase multi-sink commit, validation/test-helper reduction, shared Gaussian sampling, decoder-base extraction, and a one-shot application base remain follow-ups. The H3 README records why its bidirectional whole-clip VAE and finite synchronized lifecycle stay local until another consumer establishes the abstraction.
  • The existing scheduler/unique_consecutive behavior is unchanged because it exactly matches the pinned Diffusers oracle and checkpoint identity.

Verification

At 81c68f46143ff5a06b6640324e118fbce2999521, a clean detached checkout passed the complete pre-commit gate and the full CPU tier: 2,100 passed, 2 skipped, 346 deselected. Focused post-merge/review suites passed 222 and 107 tests, including real external-ffmpeg/ffprobe round trips. All 48/48 PR commits from current main have DCO sign-off.

No new GPU run was needed for this review batch because it does not change native H3 math, weights, scheduling, or device placement. The immutable RTX PRO artifacts/performance remain explicitly anchored to the real-weight validation head in ROLLOUT.md.

@ArielG-NV ArielG-NV left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest splitting PR for simplicity.

Reviewed everything except for integrations/integrations_v2

#517 - We plan to formalize and "make our file-structure less of a mess" - I wonder if the H3 implementation code should move into this now?

Comment on lines +312 to 318
@final
def presented_model_audio(self) -> AudioOutput | None:
"""Return the current model chunk's audio payload at most once."""
return self._presentation_manager.presented_audio()


def _contains_close(events: UserInputEvents) -> bool:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just pull out audio support from this PR, merge that with minimal demo on how to use -- then come back to this PR as a dependency (shrink the monumental amount of changes in this PR by hoisting some out into a sensible separate easier to review PR)

Comment on lines +53 to +58
@runtime_checkable
class AbortableOutputSink(OutputSink, Protocol):
"""An output sink that can discard an incomplete session atomically.

Both terminal operations must be idempotent. The runtime calls ``close``
only for a successful run. It calls ``abort`` after cancellation or failure,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class seems to be unnecessary?

  • Adds special handling for when we fail during write ==> this should just lead to the app closing (close)
  • Adds special handling for when we fail during open/close ==> If this logic is needed we can consider just adding abort()/reset() to OutputSink

Comment on lines +139 to +140
Only one channel in a model chunk may carry audio. The presentation manager
makes that payload available once, with the chunk's first presented frame.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one channel in a model chunk may carry audio.

Why? isn't the data tied to StepResult.audio. We can just... allow all layers to contain

and composite by default (BlitModelOutputToScreenLoop)

"""
width, height, fps = _probe_video_stream(path)
command = [
_find_ffmpeg_binary(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not embed ffmpeg.
maybe for something core like this we should use something we can embed in our repo like moviepy or opencv.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger, we can't distribute nor link ffmpeg . Call as separate process and users install it is lawerly OK but not ideal, I'll try to find a less encumbered alternative which doesn't itself depend on ffmpeg .

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moviepy and opencv both depend on ffmpeg .
There is an accelerated PyNvVideoCodec , but not all target NV GPUs have the enc/dec HW

Comment on lines +2 to +14
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only need the first 2 lines of the liscence

@@ -0,0 +1,31 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only need short liscence

Comment on lines +19 to +29
def add_exception_note(error: BaseException, note: str) -> None:
"""Add diagnostic context when the running Python supports exception notes.

``BaseException.add_note`` was added in Python 3.11, while FlashDreams
supports Python 3.10. Cleanup must never replace the primary failure merely
because that compatibility method is absent.
"""
add_note = getattr(error, "add_note", None)
if callable(add_note):
add_note(note)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this file/workaround?

  • we should maybe consider moving the pytoml to 3.12 - we already use it in most integrations... we just never moved some of them formally to 3.12

Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
Signed-off-by: Jonathan McCaffrey <jmccaffrey@nvidia.com>
@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

Stack split in progress. The fully validated pre-split head is 81c68f4 and is being preserved as dev/jmccaffrey/minimax-h3-integ-v2-pre-split-81c68f46. Model-neutral V2 synchronized-audio/runtime changes are moving to a new prerequisite PR. This PR will retain the H3 review thread, be rebuilt on the exact audio prerequisite head, and then be retargeted to that branch. Existing review links and GPU artifact hashes remain valid historical evidence; no H3 math, weights, scheduling, or device-placement code is changing in the split.

@jmccaffrey-nv
jmccaffrey-nv force-pushed the dev/jmccaffrey/minimax-h3-integ-v2 branch from 81c68f4 to 3cfdd23 Compare August 26, 2026 21:26
@jmccaffrey-nv
jmccaffrey-nv changed the base branch from main to dev/jmccaffrey/v2-audio-output August 26, 2026 21:26
@jmccaffrey-nv

Copy link
Copy Markdown
Collaborator Author

Stack split complete on H3 head 3cfdd23, based on audio prerequisite #526 head 264840e. The visible range is 3 signed-off H3 commits and 43 H3/input/metadata paths. Local: focused H3/media suite 138 passed; full ci_cpu 2,100 passed, 2 skipped, 346 deselected; all pre-commit hooks and all three distribution builds passed. Fresh GitHub: CPU 8m20s, GPU 16m21s, WorldLens 12m54s, OmniDreams 15m14s, LingBot 24m23s, plus docs/REUSE/OSRB—all passed. The PR remains draft and mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: MiniMax-H3 support

2 participants