Repository navigation
expose memory-related session options via config - #3503
Conversation
dosubot
Bot
added
the
size:S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 configuration options to disable memory pattern optimization and the CPU memory arena in ONNX sessions. The review feedback suggests renaming the new configuration keys from snake_case to PascalCase to maintain consistency with existing parameters in the codebase.
|
Can you tell me if I will be able to use this config in one of your language bindings (for example go) like: config.ModelConfig.Provider = "cpu:/path/to/config.txt" |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/csrc/session.cc`:
- Around line 179-195: The code currently treats any non-zero integer returned
by ToIntOrDefault for "enable_mem_pattern" and "enable_cpu_mem_arena" as
enabled; update both branches to explicitly validate that the parsed int is
either 0 or 1 (for keys "enable_mem_pattern" and "enable_cpu_mem_arena"), and if
it is outside that range call a clear failure path (e.g., log an error via your
logger and abort/throw) instead of silently proceeding; keep the existing calls
to sess_opts.DisableMemPattern() and sess_opts.DisableCpuMemArena() when the
value is 0 and still erase the config key after validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 311dcf67-f10d-446c-871c-66cbe04c4ff6
📒 Files selected for processing (1)
sherpa-onnx/csrc/session.cc
| if (config.find("enable_mem_pattern") != config.end()) { | ||
| int32_t enable_mem_pattern = | ||
| ToIntOrDefault(config["enable_mem_pattern"], 1); | ||
| if (enable_mem_pattern == 0) { | ||
| sess_opts.DisableMemPattern(); | ||
| } | ||
| config.erase("enable_mem_pattern"); | ||
| } | ||
|
|
||
| if (config.find("enable_cpu_mem_arena") != config.end()) { | ||
| int32_t enable_cpu_mem_arena = | ||
| ToIntOrDefault(config["enable_cpu_mem_arena"], 1); | ||
| if (enable_cpu_mem_arena == 0) { | ||
| sess_opts.DisableCpuMemArena(); | ||
| } | ||
| config.erase("enable_cpu_mem_arena"); | ||
| } |
There was a problem hiding this comment.
Validate memory flags strictly to avoid silent misconfiguration.
At Line 181 and Line 190, non-0/1 values are implicitly treated as enabled, so invalid config can pass silently. Please validate accepted values explicitly (0 or 1) and log/abort on invalid input.
Suggested tightening
if (config.find("enable_mem_pattern") != config.end()) {
- int32_t enable_mem_pattern =
- ToIntOrDefault(config["enable_mem_pattern"], 1);
+ int32_t enable_mem_pattern =
+ ToIntOrDefault(config["enable_mem_pattern"], -1);
+ if (enable_mem_pattern != 0 && enable_mem_pattern != 1) {
+ SHERPA_ONNX_LOGE("Invalid enable_mem_pattern: %s (expected 0 or 1)",
+ config["enable_mem_pattern"].c_str());
+ SHERPA_ONNX_EXIT(-1);
+ }
if (enable_mem_pattern == 0) {
sess_opts.DisableMemPattern();
}
config.erase("enable_mem_pattern");
}
if (config.find("enable_cpu_mem_arena") != config.end()) {
- int32_t enable_cpu_mem_arena =
- ToIntOrDefault(config["enable_cpu_mem_arena"], 1);
+ int32_t enable_cpu_mem_arena =
+ ToIntOrDefault(config["enable_cpu_mem_arena"], -1);
+ if (enable_cpu_mem_arena != 0 && enable_cpu_mem_arena != 1) {
+ SHERPA_ONNX_LOGE("Invalid enable_cpu_mem_arena: %s (expected 0 or 1)",
+ config["enable_cpu_mem_arena"].c_str());
+ SHERPA_ONNX_EXIT(-1);
+ }
if (enable_cpu_mem_arena == 0) {
sess_opts.DisableCpuMemArena();
}
config.erase("enable_cpu_mem_arena");
}📝 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.
| if (config.find("enable_mem_pattern") != config.end()) { | |
| int32_t enable_mem_pattern = | |
| ToIntOrDefault(config["enable_mem_pattern"], 1); | |
| if (enable_mem_pattern == 0) { | |
| sess_opts.DisableMemPattern(); | |
| } | |
| config.erase("enable_mem_pattern"); | |
| } | |
| if (config.find("enable_cpu_mem_arena") != config.end()) { | |
| int32_t enable_cpu_mem_arena = | |
| ToIntOrDefault(config["enable_cpu_mem_arena"], 1); | |
| if (enable_cpu_mem_arena == 0) { | |
| sess_opts.DisableCpuMemArena(); | |
| } | |
| config.erase("enable_cpu_mem_arena"); | |
| } | |
| if (config.find("enable_mem_pattern") != config.end()) { | |
| int32_t enable_mem_pattern = | |
| ToIntOrDefault(config["enable_mem_pattern"], -1); | |
| if (enable_mem_pattern != 0 && enable_mem_pattern != 1) { | |
| SHERPA_ONNX_LOGE("Invalid enable_mem_pattern: %s (expected 0 or 1)", | |
| config["enable_mem_pattern"].c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| if (enable_mem_pattern == 0) { | |
| sess_opts.DisableMemPattern(); | |
| } | |
| config.erase("enable_mem_pattern"); | |
| } | |
| if (config.find("enable_cpu_mem_arena") != config.end()) { | |
| int32_t enable_cpu_mem_arena = | |
| ToIntOrDefault(config["enable_cpu_mem_arena"], -1); | |
| if (enable_cpu_mem_arena != 0 && enable_cpu_mem_arena != 1) { | |
| SHERPA_ONNX_LOGE("Invalid enable_cpu_mem_arena: %s (expected 0 or 1)", | |
| config["enable_cpu_mem_arena"].c_str()); | |
| SHERPA_ONNX_EXIT(-1); | |
| } | |
| if (enable_cpu_mem_arena == 0) { | |
| sess_opts.DisableCpuMemArena(); | |
| } | |
| config.erase("enable_cpu_mem_arena"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/csrc/session.cc` around lines 179 - 195, The code currently
treats any non-zero integer returned by ToIntOrDefault for "enable_mem_pattern"
and "enable_cpu_mem_arena" as enabled; update both branches to explicitly
validate that the parsed int is either 0 or 1 (for keys "enable_mem_pattern" and
"enable_cpu_mem_arena"), and if it is outside that range call a clear failure
path (e.g., log an error via your logger and abort/throw) instead of silently
proceeding; keep the existing calls to sess_opts.DisableMemPattern() and
sess_opts.DisableCpuMemArena() when the value is 0 and still erase the config
key after validation.
Yes, you can do that after this PR gets merged. |
|
Nice, thank you for the confirmation and the help earlier! |
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
2 participants
As discussed here #3500
Summary by CodeRabbit