fix(packing): gate timestep-embedding fast path on timestep values, not tensor shape - #236
Conversation
uses_single_timestep is documented as "whether all noised tokens share one input timestep scalar" but was computed as input_timesteps.numel() == 1, a property of the tensor's shape rather than its values. The fast path in _embed_packed_timesteps was therefore skipped for any uniform batch above size one, for the [2B] tensor built by use_batched_cfg, and for single-sample diffusion forcing with a uniform [1, T] schedule. Derive the flag from the timestep values. pack_input_sequence already rejects CUDA input, so this reads a CPU tensor and adds no device synchronization. Because training_step overwrites the packed action and sound timesteps with per-sample sigmas after packing, under independent_action_schedule and independent_sound_schedule, both rewrite sites now AND in the uniformity of what they wrote. The previous numel() == 1 derivation masked this, since a single-element tensor implies batch size 1, which makes every modality trivially uniform. Adds packers_test.py, the first coverage for this flag. Signed-off-by: Umair <16253819+umairjavaid@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change aligns the flag with its documented semantics, preserves safety for post-pack timestep rewrites, and adds focused tests that reproduce the prior failures and validate the corrected behavior.
Pull request overview
This PR fixes the PackedSequence.uses_single_timestep gate so it matches its documented contract (“all noised tokens share one timestep scalar”) by checking timestep values rather than the timestep tensor’s shape, ensuring the timestep-embedding broadcast fast path is used for uniform batches across more real inference/training shapes.
Changes:
- Add
uses_single_timestep(input_timesteps)helper that returnsTrueiff all timestep values are identical (andFalsefor empty tensors / NaNs). - Switch
pack_input_sequenceto derivePackedSequenceBuilder(uses_single_timestep=...)from the helper rather thannumel() == 1. - Ensure
training_stepclears (ANDs) the flag when action/sound timesteps are overwritten post-pack under independent schedules, and add a new targeted test suite covering positive/negative cases and the “post-pack rewrite” composition.
File summaries
| File | Description |
|---|---|
cosmos_framework/model/generator/omni_mot_model.py |
ANDs uses_single_timestep with rewritten per-sample action/sound schedules so the embedding fast path remains correct after post-pack timestep overrides. |
cosmos_framework/data/generator/sequence_packing/packers.py |
Introduces value-based uses_single_timestep helper and uses it to initialize the packed-sequence flag during packing. |
cosmos_framework/data/generator/sequence_packing/packers_test.py |
Adds regression tests covering uniform vs non-uniform timestep inputs and the independent-modality post-pack invalidation behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Kindly tag reviewers for this PR |
|
Merged — thank you for this contribution! Really appreciated the thoroughness here: the audit of every post-pack writer of |
Fixes #235.
Summary
uses_single_timestepgates the one-row-and-broadcast fast path in_embed_packed_timesteps. Its docstring describes a property of timestep values ("all noised tokens share one input timestep scalar",sequence.py:42) but it was computed asinput_timesteps.numel() == 1, a property of the tensor's shape. The fast path was therefore skipped whenever the timesteps were uniform but the tensor held more than one element.This makes the gate implement its own documented contract, and keeps it honest for the one code path that rewrites timesteps after packing.
[0.5]— one sample[0.5, 0.5]— batch sharing a timestepzeros(2B)—use_batched_cfgfull((1, T), 0.5)— uniform diffusion forcing[0.25, 0.75]— per-sample sigmas[[0.1, 0.2, 0.3, 0.4]]— per-frame sigmasThe three regressed rows include every step of the template inference path, where
_copy_timestep_to_template(omni_mot_model.py:2734) broadcasts a single scalar over all noisy tokens, so uniformity is guaranteed by construction.The fix, in two parts
1. Derive the flag from the values (
packers.py). Extracted as a small helper because the condition is now needed in more than one place.pack_input_sequencealready rejects CUDA input (packers.py:193), so this reads a CPU tensor and adds no device synchronization:Empty input returns
False, byte-identical to the previous behaviour.NaNnever compares equal, so it declines the fast path.2. Keep the flag honest when timesteps are rewritten after packing (
omni_mot_model.py). This is what makes a values check safe rather than merely correct-looking. Underindependent_action_schedule/independent_sound_schedule,training_stepreplaces the packed action and sound timesteps with one sigma per sample after the packer has already set the flag from the vision timesteps. A uniform vision batch with differing action sigmas would otherwise leave the flag true and let_embed_packed_timestepsbroadcast sample 0's action sigma across the whole batch. The oldnumel() == 1derivation masked this, since a single-element tensor implies batch size 1, which makes every modality trivially uniform. Both rewrite sites now AND in the uniformity of what they wrote:To be explicit about reachability: training sigmas are independent continuous draws per sample (
rectified_flow.py), so a uniform vision batch atB > 1is essentially unreachable with today's sampler. Part 2 is defensive — it makes the flag's contract hold for any caller (a fixed-sigma evaluation, an external packer) rather than by accident of the sampler.I audited every writer to a packed
.timestepsfield._copy_timestep_to_template(expand_as),omni_mot_causal_model.py:2885(timestep.flatten()[0].repeat(n)) andomni_mot_causal_model.py:3090(torch.zeros) are uniform by construction;temporal_causal.py:284derives each frame's value from the same per-sampleinput_timestepthe packer-level check already covers;teacher_forcing.py:30writes an empty tensor, which the consumer'snumel() > 1guard already handles. The two independent-schedule sites were the only ones that could produce non-uniform values.Tests
New
cosmos_framework/data/generator/sequence_packing/packers_test.py— the first coverage for this flag. Construction mirrorscreate_packed_sequenceincontext_parallel_test.py. Restricted to the tests that use only the public API:mainThe five that already passed are the negative cases, which pin that the fix does not over-enable the fast path. The full file is 17 tests, covering the helper directly, the packer end to end, and the post-packing composition from part 2. CPU-only, ~3 s:
One test asserts the underlying invariant rather than the flag: when the flag is true, every packed timestep really does equal
timesteps[:1], which is what the consumer broadcasts. That holds regardless of how the flag is derived.Numerics
The fast path is not bit-identical to the general path, because an
M=1GEMM selects a different kernel thanM=Nand accumulation order differs:TimestepEmbeddingmodule, measured on thediffusersCosmos3-Edge path)This is pre-existing behaviour: the fast path is already taken at batch size 1 on
main, so this change extends an already-accepted approximation to more shapes rather than introducing a new class of difference. Flagging it explicitly because outputs forB > 1will no longer be bit-identical to before this patch.For contrast, forcing the flag true on genuinely mixed timesteps (a synthetic check) produces a max abs difference of 0.103 — four orders of magnitude larger, which is the wrong-conditioning error part 2 prevents.
Performance
The saving is confined to the conditioning path: the fp32 MLP runs on 1 row instead of one per noisy token. Large factor on the module in isolation, but well under one percent of a full generator forward in bf16, so I would frame this as making the gate match its documented contract, with the
use_batched_cfginteraction as the concrete motivation, rather than as a speedup.For scale, the equivalent module at equivalent shapes — measured on the
diffusersCosmos3-Edge path at 480p/61 frames, not in this repo — evaluates 6,240 rows where 1 suffices.Notes
torch.compile: the value check runs in the packer, outside any compiled region, so it cannot introduce a graph break. The flag is consumed inside_encode_vision/_encode_action, whichapply_compilecompiles individually (parallelize_vfm_network.py:37-41); there it is a Python bool that Dynamo already guards on today. In inference the flag is now constantTrueacross batch sizes, so this removes a guard variant rather than adding one.ruff checkandruff format --checkproduce identical output tomainfor both edited files (the two pre-existingI001findings are left untouched to keep the diff focused); the new test file is clean under both.CONTRIBUTING.md.Follow-up, deliberately not in this PR
uses_single_timesteplives on thePackedSequencebut is consumed per modality — vision, action and sound each get their own_embed_packed_timestepscall while sharing one flag. That is why part 2 needs an AND rather than a per-modality answer, and it means a uniform-vision batch with non-uniform action sigmas now conservatively loses the fast path for vision too. Moving the flag onto the modality would fix that and delete the AND. Happy to do it as a separate change if you'd like it.