-
Notifications
You must be signed in to change notification settings - Fork 903
Fix GigaAM transducer encoder output length data type #2426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
""" WalkthroughThis change updates the decoding logic in the offline transducer NeMo decoder to support both Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Decoder
participant EncoderOutLenTensor
User->>Decoder: Call Decode()
Decoder->>EncoderOutLenTensor: Check element type
alt int32
Decoder->>EncoderOutLenTensor: Access data as int32_t
else int64
Decoder->>EncoderOutLenTensor: Access data as int64_t
else unsupported
Decoder->>Decoder: Log error and exit
end
Decoder->>Decoder: Perform decoding loop (formatting adjusted)
Decoder-->>User: Return decoded results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sherpa-onnx/csrc/offline-transducer-greedy-search-nemo-decoder.cc
(2 hunks)
🔇 Additional comments (3)
sherpa-onnx/csrc/offline-transducer-greedy-search-nemo-decoder.cc (3)
65-92
: LGTM! Formatting improvements enhance readability.The indentation and spacing corrections improve code readability without altering the existing logic or control flow.
107-114
: Excellent type safety implementation.The addition of explicit type checking for
encoder_out_length
tensor properly addresses the data type compatibility issue. The error handling provides clear diagnostics and follows good practices with appropriate logging and program termination.
123-125
: Correct implementation of type-safe conditional access.The conditional logic properly handles both
int32_t
andint64_t
data types using the detectedlength_type
. The implementation is clean and ensures type safety when accessing the tensor data.
sherpa-onnx/csrc/offline-transducer-greedy-search-nemo-decoder.cc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a data type compatibility issue in the GigaAM transducer encoder where the output length tensor could be either int32 or int64, but the code only handled int32. The fix adds runtime type checking and supports both data types.
- Adds runtime detection of encoder output length tensor data type (int32 vs int64)
- Implements conditional data access based on the detected type
- Adds error handling for unsupported data types
std::vector<OfflineTransducerDecoderResult> ans(batch_size); | ||
|
||
for (int32_t i = 0; i != batch_size; ++i) { | ||
const float *this_p = p + dim1 * dim2 * i; | ||
int32_t this_len = p_length[i]; | ||
int32_t this_len = length_type == ONNX_TENSOR_ELEMENT_DATA_TYPE_INT32 | ||
? encoder_out_length.GetTensorData<int32_t>()[i] | ||
: encoder_out_length.GetTensorData<int64_t>()[i]; |
Copilot
AI
Jul 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tensor data is being accessed inside the loop for each iteration. Consider caching the tensor data pointer outside the loop to avoid repeated GetTensorData calls.
Copilot uses AI. Check for mistakes.
Fixes #2420
Summary by CodeRabbit
Bug Fixes
Style