Keep CTC alignment inside each item's length - #358
Conversation
A shorter row was scored through the padded frames, so a repeated label landed one frame early.
LauraGPT
left a comment
There was a problem hiding this comment.
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.
Add an unequal-length batch test and a CPU-lengths test for an accelerator, and run tests.test_ctc_alignment in the container workflow.
|
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). |
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.
|
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. |
|
Rechecked On Python 3.12.3 / the workflow-pinned torch 2.12.1+cpu, 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 |
LauraGPT
left a comment
There was a problem hiding this comment.
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.
|
Thanks for the recheck and for merging it. The unequal-length fixture is the one that catches the padded frame. |
Summary
ctc_forced_alignof 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 second1moved. 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.pyfails on the current aligner ([1, 0, 0, 1, 0]) and passes after the change, 9 tests