Fix modelconverter analyze command - #271
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe analysis command now limits supported input files per directory. ChangesRVC4 analysis workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RVC4Analyzer
participant SNPE_DLC
participant ONNX_Session
participant Comparison_CSV
RVC4Analyzer->>RVC4Analyzer: Prepare one supported sample
RVC4Analyzer->>SNPE_DLC: Execute DLC with output directory
SNPE_DLC-->>RVC4Analyzer: Return inference outputs
RVC4Analyzer->>ONNX_Session: Collect comparison statistics
ONNX_Session-->>RVC4Analyzer: Return statistics
RVC4Analyzer->>Comparison_CSV: Write aggregated results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
=========================================
+ Coverage 0 99.98% +99.98%
=========================================
Files 0 73 +73
Lines 0 5101 +5101
=========================================
+ Hits 0 5100 +5100
- Misses 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modelconverter/packages/rvc4/analyze.py (1)
143-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
typeparameter.
typeshadows the Python builtin and raises an A002 error. Rename it todtype, update the annotation, and update theastype(...)call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/packages/rvc4/analyze.py` around lines 143 - 180, Rename the prepare-raw-inputs method parameter type to dtype, update its type annotation, and replace both astype(type) usages with astype(dtype). Preserve the existing input conversion behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelconverter/packages/rvc4/analyze.py`:
- Around line 35-38: Update the analyzer initialization around self.image_subset
and Analyzer.__init__ so image_subset is assigned before base initialization.
Move or override the base size validation to count only paths accepted by
_prepare_input_matcher and restricted by image_subset, while preserving the
existing device setup and handler creation flow.
- Around line 114-118: Update the input-path comprehension in the analyzer to
require path.is_file() in addition to the supported suffix check, ensuring
directories with input-like extensions are excluded while valid files remain
included.
---
Nitpick comments:
In `@modelconverter/packages/rvc4/analyze.py`:
- Around line 143-180: Rename the prepare-raw-inputs method parameter type to
dtype, update its type annotation, and replace both astype(type) usages with
astype(dtype). Preserve the existing input conversion behavior.
🪄 Autofix
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 Plus
Run ID: c3940ca7-d214-4d0a-9c17-bd9d61e3932c
📒 Files selected for processing (2)
modelconverter/__main__.pymodelconverter/packages/rvc4/analyze.py
klemen1999
left a comment
klemen1999
left a comment
There was a problem hiding this comment.
I'd propose a better flow overall. If I understand the code as is now correctly we are doing:
For each sample:
try to guess the output structure
if it fails parse through SNPE errors and create missing folders
run snpe-net-run to create outputs
copy outputs to host
delete everything
But since we are dealing with the same model and we are just changin the inputs between samples I think a better flow would be:
one-time setup:
push DLC
create the dir structure on the device including output directories (use SNPE with a dummy/first input directly, don't need to even try to guess it with ONNX)
for each sample:
replace staged input files and input_list.txt
delete old output .raw files, but keep their parent directories
run SNPE
pull outputs to the host
compare them locally
delete local temporary output files
This way we are technically doing one more inference (the first dummy one) but we don't need to keep trying and guessing the structure for every sample.
Does this make sense?
|
For CI fixes we need to merge in |
…the device workspace and push the DLC once at the beginning)
klemen1999
left a comment
klemen1999
left a comment
There was a problem hiding this comment.
Generally LGTM, just two smaller mistakes
3 participants
Purpose
Order of fixes performed:
snpe-net-run --debugwould use was "predicted" incorrectly. Solution: parse SNPE's missing-utput error, create the parent directory on-device then retry.--output-dirshould have been passed tosnpe-net-run(in the other RVC4 paths it is)--image subset <num-images>parser argument to optionally limit the number of images used in the analyze loop.Working usage now:
modelconverter analyze --dlc-model-path <dlc-model-path>.dlc --onnx-model-path <onnx-intermediate-path>.onnx --image-dirs <flat-dir-of-images> --device-ip <device-ip> --image-subset NSpecification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
Summary by CodeRabbit
New Features
Bug Fixes
Documentation