Skip to content

Keep CTC alignment inside each item's length - #358

Merged
LauraGPT merged 3 commits into
QwenAudio:mainfrom
SashaMIT:codered-ctc-length
Sep 30, 2026
Merged

LauraGPT merged 3 commits into
QwenAudio:mainfrom
SashaMIT:codered-ctc-length

Conversation

@SashaMIT

Copy link
Copy Markdown
Contributor

Summary

ctc_forced_align of a 5-frame utterance stored in a 6-frame tensor returned [1, 0, 0, 1, 0]. The extra blank frame was still part of the score, so the second 1 moved. Those five frames now stay [1, 0, 0, 0, 1]. A tensor whose time size matches the utterance still returns that alignment. Ignore ids are replaced on a copy, so the caller's targets stay put.

Test plan

  • python -m unittest tests/test_ctc_alignment.py tests/test_model_timestamps.py fails on the current aligner ([1, 0, 0, 1, 0]) and passes after the change, 9 tests

A shorter row was scored through the padded frames, so a repeated label landed one frame early.

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The length fix and non-mutation change reproduce as intended: the two unittest modules pass all 9 tests on this head, while the same tests against base ea15219 fail the padded-frame and target-mutation cases (2 failures, 7 passes). I also checked 120 small valid CTC examples against an exhaustive path-score oracle, each cropped and padded (240 checks), plus 30 unequal-length batches against their individually cropped items (60 comparisons); the head passes both selections. These checks used CPU tensors with Python 3.12 / PyTorch 2.14.0, not model weights or hardware inference.

Please address the mixed-device regression below before merging. Also add a direct unequal-length batch regression to the committed tests, and include tests.test_ctc_alignment in the container workflow's model-contract command and path filters: currently that workflow runs only tests.test_model_timestamps, whose batch case calls the aligner one cropped item at a time.

Device limitation: the complete aligner was not run on GPU. The finding is based on the changed device flow, independent source review, and an isolated metadata-only torch.where check (CPU mask + meta-device scores rejects; same-device control succeeds). A whole-function FakeTensor probe was inconclusive because both revisions encountered a pre-existing data-dependent scalar conversion, so it is not counted as a device regression reproduction.

Comment thread utils/ctc_alignment.py
Add an unequal-length batch test and a CPU-lengths test for an accelerator, and run tests.test_ctc_alignment in the container workflow.
@SashaMIT

Copy link
Copy Markdown
Contributor Author

Thank you for the careful review and for checking against an exhaustive oracle. The mixed-device case reproduces here on MPS: CPU input lengths with MPS emissions raised the same-device error on the previous head, and the base accepted that call.

Both frame masks now use one copy of the lengths on the score device. I added the direct unequal-length batch test, which compares each item with its own cropped call, and a test with CPU input lengths and accelerator emissions (CUDA or MPS, skipped when neither exists). tests.test_ctc_alignment now runs in the container workflow's model-contract command and both path filters. Pushed as 837ed1e. On CPU the timestamp and alignment suites pass together (11 tests).

The random batch agreed with a cropped item on the previous code. It now uses the five-frame example that moves when a padded frame is included.
@SashaMIT

Copy link
Copy Markdown
Contributor Author

You are right that the accelerator case skips on CPU, so that command is 10 passed and 1 skipped, not 11 passes. The unequal-length batch test now uses the five-frame example, which disagrees with a cropped item on the old aligner and agrees here.

@LauraGPT

Copy link
Copy Markdown
Member

Rechecked e3881391e47ba72129e9eea86ce78f819eef6d88. The deterministic unequal-length fixture now catches the intended regression: the new test fails with the base ea152195 aligner ([1, 0, 0, 1, 0] versus cropped [1, 0, 0, 0, 1]) and passes with this head.

On Python 3.12.3 / the workflow-pinned torch 2.12.1+cpu, python -m unittest tests.test_model_timestamps tests.test_ctc_alignment -v collected 11 tests: 10 passed, 1 skipped because no accelerator was available. The changed batch test was also run individually against both aligners. Source-byte checks confirm that production code and the workflow are unchanged from 837ed1ee; only the test fixture changed. The earlier non-blocking test-strengthening suggestion is addressed, with no new P1/P2 found in this bounded review.

This is CPU test evidence, not a full model or container run, nor independent CUDA/MPS validation. The existing Torch-only test environment emitted its missing-NumPy initialization warning; these checks do not exercise NumPy conversion. Hosted container run 36581049329 requires upstream authorization and is not a passing CI result. No workflow approval, merge or image publication was performed.

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed exact head e3881391e47ba72129e9eea86ce78f819eef6d88 against current main ea15219509625e5d4c5143c37c86970135886b5d.

The final deterministic unequal-length fixture now directly covers the padded-frame regression. The submitted head and a clean synthetic merge each run 10 passed, 1 skipped (needs an accelerator) across tests.test_model_timestamps and tests.test_ctc_alignment; compileall and git diff --check also pass. Source review confirms that the length mask stays on the score device and callers' targets are no longer mutated, addressing the earlier blocking review.

Approving. The accelerator case remains an explicit environment skip, not a claimed hardware pass.

@LauraGPT
LauraGPT merged commit 586bbdb into QwenAudio:main Sep 30, 2026
@SashaMIT

Copy link
Copy Markdown
Contributor Author

Thanks for the recheck and for merging it. The unequal-length fixture is the one that catches the padded frame.

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.

2 participants