Conversation
…tions Inside the per-session loop of SpeakerTaggedASR._add_speaker_transcriptions, the word and segment loops read trans_hyp[0].timestamp while the rest of the loop uses trans_hyp[sess_idx]. Every session after the first was therefore paired with session 0's words and segments: it raised a "Word mismatch" ValueError or an IndexError when the transcripts differ, and when they share the same words it silently took session 0's timestamps and overwrote session 0's segment speaker labels in place. Index by sess_idx in both loops. A single session is unchanged. Add a unit test with two and three sessions that fails without this change. Signed-off-by: Zaheer Sheriff K <zaheersheriff.k@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
Makes
SpeakerTaggedASR._add_speaker_transcriptionsread each session's own word and segment timestamps. At the moment it reads session 0's for every session.Collection: ASR
Changelog
nemo/collections/asr/parts/utils/multispk_transcribe_utils.py,_add_speaker_transcriptions:trans_hyp[0]→trans_hyp[sess_idx]in the word loop (L998) and the segment loop (L1004).tests/collections/speaker_tasks/utils/test_spk_tagged_asr_utils.py: a newTestAddSpeakerTranscriptionsclass with one test, parametrized over two two-session cases and one three-session case.The problem
Inside
for sess_idx, (uniq_id, _) in enumerate(test_manifest_dict.items()), lines 996, 1002 and 1018 usetrans_hyp[sess_idx]. The two loops that read the timestamps use index 0:trans_hypis a flat list with one hypothesis per session.merge_transcript_and_speakersbuildsword_and_ts_seq[uniq_id]['words']for sessionidxfromasr_hypotheses[idx](L846), and_add_speaker_transcriptionsthen indexes it byword_idxandw_count. Index 0 is therefore session 0, not a nested per-session container, and sessioniends up with session 0's words carrying sessioni's speaker labels.I checked this with a CPU script that passes hand-built hypotheses and diarization tensors through the real
merge_transcript_and_speakers→_add_speaker_transcriptionspath. No model is involved. Session 0 issess_aand session 1 issess_b:mainValueError: Word mismatch: 'hello' != 'good' at session 1, word count 0.IndexError: list index out of rangeThe script's output for the third case, exactly as printed apart from the
#lines I added:On
main, session 0's segment speaker changes fromspeaker_0tospeaker_1because the segment loop writestrans_segdict['speaker']into the dicts it iterates over, and onmainthose dicts belong to session 0.With this change all three cases give each session its own timestamps and speakers. A single session is not affected, because
sess_idxis always 0. The script's single-session output is identical before and after (compared as JSON).The script is not part of this PR. The new test covers the same three situations by calling
_add_speaker_transcriptionsdirectly, so it serves as the runnable reproduction.Impact, stated honestly
This is a latent defect in a helper, and nothing in the repository reaches it today. The only caller is
SpeakerTaggedASR.perform_offline_stt_spk(L1043). Neitherspeech_to_text_multitalker_streaming_infer.pynor the multitalker tutorial calls that method; both use the streaming paths.On current
main,perform_offline_stt_spkfailed before reaching this function in every case I tried:best_hyp, _ = transcriptions(L1036). That matches the(hypotheses, all_hypotheses)tuple thatEncDecRNNTModel._transcribe_output_processingreturned before changed asr models outputs to be consistent #11818; it now returns a list. I built a small, randomly initialisedEncDecRNNTBPEModellocally, with a SentencePiece tokenizer trained on a few lines of text and noise audio. Itstranscribe()returns a flatlistofHypothesis, and with a stub diarizer:ValueError: not enough values to unpack (expected 2, got 1);ValueError: too many values to unpack (expected 2);merge_transcript_and_speakers, whereasr_hypotheses[idx]raisesTypeError: 'Hypothesis' object is not subscriptable(L841).self.cfg.dataset_manifesttotranscribe()(L1033). The example script'sMultitalkerTranscriptionConfighasmanifest_filebut nodataset_manifest, so accessing it raisesConfigAttributeError.I did not run the released multitalker checkpoints. From reading the code,
EncDecMultiTalkerRNNTBPEModeloverrides_transcribe_forwardand_setup_transcribe_dataloaderbut nottranscribeor_transcribe_output_processing, so I expect it to return the same list.I kept this PR to the indexing fix. Whether the offline entry point should be updated to the current
transcribe()output or removed is for you to decide, and I'm happy to follow up either way. From reading the code, if it is updated, this fix is needed for any manifest with more than one file.Tests
The new test is CPU-only and needs no model, tokenizer or download. It calls
_add_speaker_transcriptionsdirectly with aSimpleNamespaceasself, the way the existing tests callperform_parallel_streaming_stt_spk, and asserts that each session's word and segment timestamps and speakers come back as its own. The cases:different_words: onmainthis hits theValueErrorabove.same_words_different_times: onmainthis runs without error but returns the wrong values. The first mismatch is session 0's segment speaker,speaker_1instead ofspeaker_0.three_sessions_fewer_words: session 1 has one word and session 0 has three, so onmainthis hits theIndexErrorabove. The third session also makes the test reject an index that agrees withsess_idxonly for the first two sessions: I checkedtrans_hyp[-sess_idx], which passes the two two-session cases and fails this one.Fixing only one of the two lines still fails all three cases.
The 12 errors are the same tests in both runs, and each one is
fixture 'test_data_dir' not found.tests/conftest.pydownloads the test-data archive, and I ran with--noconftestto avoid that download (registering conftest's three markers so--strict-markersstill applied). These tests use the RNNTasr_modelfixture fromtest_asr_rnnt_encoder_model_bpe.py, whose tokenizer comes from that archive, so they could not be set up here. None of them reaches the changed code: undertests/,_add_speaker_transcriptionsandperform_offline_stt_spkare referenced only by the new class.black24.10.0 andisort5.13.2 clean at line length 119.Before your PR is "Ready for review"
Pre checks:
PR Type:
Additional Information
trans_hyp[0]lines date from the file's first version in Merge updates of Multi-Talker Parakeet Model, Modules, Dataloader and Utils PR 01 #14905._add_speaker_transcriptions. Draft [Streaming SpeechLM] Add audomodel support #16045, which targets a feature branch rather thanmain, modifies this file and its test file in other places; both its head and its base branch still have the same twotrans_hyp[0]lines.