Fix bugs in NeMo transducer modified beam search.#3589
Conversation
dosubot
Bot
added
the
size:M
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes an infinite loop bug in the NeMo transducer modified beam search decoder by introducing per-hypothesis emission tracking with a per-frame symbol cap, forcing blank transitions when the cap is reached. A minor return-type fix for a deleted copy assignment operator in the Paraformer recognizer is also included. ChangesNeMo Modified Beam Search Decoder - Per-Hypothesis Symbol Emission Tracking
Paraformer Copy Assignment Operator Signature Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to limit the number of symbols emitted per frame in the NeMo transducer decoder, preventing potential infinite loops. It also fixes the return type of a deleted assignment operator in the Paraformer implementation. The review feedback suggests defining the hardcoded symbol limit as a constexpr and optimizing performance by using std::move to avoid redundant deep copies of hypothesis states.
|
|
||
| int32_t vocab_size = model_->VocabSize(); | ||
| int32_t blank_id = vocab_size - 1; // NeMo models have blank at the end | ||
| int32_t max_symbols_per_frame = 10; |
There was a problem hiding this comment.
The max_symbols_per_frame value is hardcoded. It would be better to make this a configurable parameter in the decoder options to allow users to tune the behavior for different models. At the very least, it should be marked as constexpr to indicate it is a constant.
| int32_t max_symbols_per_frame = 10; | |
| constexpr int32_t kMaxSymbolsPerFrame = 10; |
| NeMoHypothesis new_hyp; | ||
| new_hyp.ys = hyp.ys; | ||
| new_hyp.timestamps = hyp.timestamps; | ||
| new_hyp.durations = hyp.durations; | ||
| new_hyp.ys_probs = hyp.ys_probs; | ||
| new_hyp.context_state = hyp.context_state; | ||
| new_hyp.allocator = allocator; | ||
| new_hyp.log_prob = hyp.log_prob; | ||
| new_hyp.num_symbols = 0; | ||
| new_hyp.decoder_states.reserve(hyp.decoder_states.size()); | ||
| for (const auto &state : hyp.decoder_states) { | ||
| new_hyp.decoder_states.push_back(Clone(allocator, &state)); | ||
| } | ||
| new_hyp.frame_offset = hyp.frame_offset + 1; | ||
| all_candidates.emplace_back(new_hyp.log_prob, std::move(new_hyp)); | ||
| continue; |
There was a problem hiding this comment.
The manual copying of NeMoHypothesis fields and deep copying of decoder_states is redundant and inefficient. Since hyp is not used again in this iteration after the continue, you can use std::move(hyp) to avoid expensive cloning of ONNX tensors. This significantly improves performance as it avoids memory allocations and data copying for the decoder states.
| NeMoHypothesis new_hyp; | |
| new_hyp.ys = hyp.ys; | |
| new_hyp.timestamps = hyp.timestamps; | |
| new_hyp.durations = hyp.durations; | |
| new_hyp.ys_probs = hyp.ys_probs; | |
| new_hyp.context_state = hyp.context_state; | |
| new_hyp.allocator = allocator; | |
| new_hyp.log_prob = hyp.log_prob; | |
| new_hyp.num_symbols = 0; | |
| new_hyp.decoder_states.reserve(hyp.decoder_states.size()); | |
| for (const auto &state : hyp.decoder_states) { | |
| new_hyp.decoder_states.push_back(Clone(allocator, &state)); | |
| } | |
| new_hyp.frame_offset = hyp.frame_offset + 1; | |
| all_candidates.emplace_back(new_hyp.log_prob, std::move(new_hyp)); | |
| continue; | |
| NeMoHypothesis new_hyp = std::move(hyp); | |
| new_hyp.num_symbols = 0; | |
| new_hyp.frame_offset += 1; | |
| all_candidates.emplace_back(new_hyp.log_prob, std::move(new_hyp)); | |
| continue; |
1 participant
Fixes #3588
cc @MingxinLiang
Summary by CodeRabbit
Release Notes
New Features
Refactor