Repository navigation
[feature] add FunASRNano config into golang api - #2974
Conversation
dosubot
Bot
added
the
size:M
|
Warning Rate limit exceeded@ilibx has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @ilibx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the Go API by adding comprehensive support for the FunASRNano model. It introduces a dedicated configuration structure for FunASRNano, integrates it into the existing offline model configuration, and provides the necessary C-binding logic for both initialization and memory management. This enhancement allows users to configure and utilize the FunASRNano model with its specific parameters directly through the Go interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/go/sherpa_onnx.go
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/go/sherpa_onnx.go (1)
sherpa-onnx/c-api/cxx-api.h (1)
OfflineFunASRNanoModelConfig(286-298)
🔇 Additional comments (3)
scripts/go/sherpa_onnx.go (3)
501-501: LGTM!The field addition follows the established pattern and naming conventions.
600-610: LGTM!The C struct population follows the established pattern. All 11 fields are correctly populated with appropriate type conversions, and memory management is consistent with the cleanup in
freeCOfflineRecognizerConfig.
743-776: LGTM!The cleanup logic correctly frees all 7 string fields allocated during config construction. The nil checks and pointer resets follow best practices and are consistent with the rest of the file.
| type OfflineFunASRNanoModelConfig struct { | ||
| EncoderAdaptor string | ||
| LlmPreFill string | ||
| LlmDecoder string | ||
| Embedding string | ||
| Tokenizer string | ||
| SystemPrompt string | ||
| UserPrompt string | ||
| MaxNewTokens int | ||
| Temperature float32 | ||
| TopP float32 | ||
| Seed int32 | ||
| } |
There was a problem hiding this comment.
Type inconsistency: MaxNewTokens should be int32.
The MaxNewTokens field is declared as int, but according to the C API reference (line 297 in cxx-api.h), it should be int32_t. For consistency with the C API and with the Seed field (which is correctly typed as int32), change MaxNewTokens to int32.
🔎 Proposed fix
type OfflineFunASRNanoModelConfig struct {
EncoderAdaptor string
LlmPreFill string
LlmDecoder string
Embedding string
Tokenizer string
SystemPrompt string
UserPrompt string
- MaxNewTokens int
+ MaxNewTokens int32
Temperature float32
TopP float32
Seed int32
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type OfflineFunASRNanoModelConfig struct { | |
| EncoderAdaptor string | |
| LlmPreFill string | |
| LlmDecoder string | |
| Embedding string | |
| Tokenizer string | |
| SystemPrompt string | |
| UserPrompt string | |
| MaxNewTokens int | |
| Temperature float32 | |
| TopP float32 | |
| Seed int32 | |
| } | |
| type OfflineFunASRNanoModelConfig struct { | |
| EncoderAdaptor string | |
| LlmPreFill string | |
| LlmDecoder string | |
| Embedding string | |
| Tokenizer string | |
| SystemPrompt string | |
| UserPrompt string | |
| MaxNewTokens int32 | |
| Temperature float32 | |
| TopP float32 | |
| Seed int32 | |
| } |
🤖 Prompt for AI Agents
In scripts/go/sherpa_onnx.go around lines 455 to 467, the struct field
MaxNewTokens is declared as int but the C API expects int32_t; change the
MaxNewTokens type to int32 to match the C API and keep consistency with the Seed
field, updating any usages or conversions where this field is set or passed to
ensure proper int32 handling.
![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 adds the configuration for FunASRNano to the Go API. The changes are well-contained and follow the existing structure for adding new model configurations. I've identified a minor type inconsistency in the new configuration struct and an opportunity to refactor some repetitive code to improve maintainability. Overall, the changes look good.
| Tokenizer string | ||
| SystemPrompt string | ||
| UserPrompt string | ||
| MaxNewTokens int |
There was a problem hiding this comment.
For consistency with the Seed field (which is int32) and the corresponding C++ type int32_t, it's better to define MaxNewTokens as int32 instead of int. This ensures type safety and avoids potential issues on different architectures where int might have a different size.
| MaxNewTokens int | |
| MaxNewTokens int32 |
| if c.model_config.funasr_nano.encoder_adaptor != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.encoder_adaptor)) | ||
| c.model_config.funasr_nano.encoder_adaptor = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.llm_prefill != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.llm_prefill)) | ||
| c.model_config.funasr_nano.llm_prefill = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.llm_decode != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.llm_decode)) | ||
| c.model_config.funasr_nano.llm_decode = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.embedding != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.embedding)) | ||
| c.model_config.funasr_nano.embedding = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.tokenizer != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.tokenizer)) | ||
| c.model_config.funasr_nano.tokenizer = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.system_prompt != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.system_prompt)) | ||
| c.model_config.funasr_nano.system_prompt = nil | ||
| } | ||
|
|
||
| if c.model_config.funasr_nano.user_prompt != nil { | ||
| C.free(unsafe.Pointer(c.model_config.funasr_nano.user_prompt)) | ||
| c.model_config.funasr_nano.user_prompt = nil | ||
| } |
There was a problem hiding this comment.
This block of code for freeing C strings is quite repetitive. You can refactor it to reduce duplication and improve maintainability by iterating over a slice of pointers to the string fields.
stringFields := []*(*C.char){
&c.model_config.funasr_nano.encoder_adaptor,
&c.model_config.funasr_nano.llm_prefill,
&c.model_config.funasr_nano.llm_decode,
&c.model_config.funasr_nano.embedding,
&c.model_config.funasr_nano.tokenizer,
&c.model_config.funasr_nano.system_prompt,
&c.model_config.funasr_nano.user_prompt,
}
for _, field := range stringFields {
if *field != nil {
C.free(unsafe.Pointer(*field))
*field = nil
}
}
| MaxNewTokens int | ||
| Temperature float32 | ||
| TopP float32 | ||
| Seed int32 |
There was a problem hiding this comment.
Please use int, not int32 in Go.
|
Thank you for your contribution! |
2 participants
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.