Tests refactor - #263
Open
kozlov721 wants to merge 67 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR consolidates CI workflows, adds Docker development support, introduces layout-aware preprocessing, updates conversion behavior, replaces legacy package tests with host-side unit tests, and adds broad cross-platform conversion, precision, archive, calibration, evaluation, and type coverage. ChangesCI and development environment
Runtime conversion behavior
Conversion and evaluation coverage
Host-side unit tests
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
kozlov721
force-pushed
the
tests/improvement
branch
from
July 25, 2026 19:26
f295733 to
c0f6bf2
Compare
kozlov721
requested review from
conorsim,
klemen1999 and
tersekmatija
and removed request for
a team
July 26, 2026 21:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelconverter/packages/hailo/exporter.py (1)
240-256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLog configured normalization parameters ignored for non-3-channel Hailo inputs.
In
HailoExporter._get_alls,inp.scale_valuesandinp.mean_valuesare only read forvalues_len == 3; for 1-channel inputs the configured preprocessing is dropped silently. Add a warning when these values are present but skipped, so users know which inputs are affected.🤖 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/hailo/exporter.py` around lines 240 - 256, The HailoExporter._get_alls normalization branch silently skips configured scale_values or mean_values for non-3-channel inputs. Add a warning in the values_len != 3 path when either preprocessing setting is present, identifying the affected input, while preserving the existing behavior of omitting normalization.
🧹 Nitpick comments (1)
pyproject.toml (1)
152-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMeasure coverage for production code only.
Including
testsinsourcemakes test and fixture implementation part of the coverage denominator, so the reported percentage no longer representsmodelconverterconversion coverage. Test execution is still recorded when only the package is a source.Proposed change
[tool.coverage.run] -source = ["modelconverter", "tests"] +source = ["modelconverter"] relative_files = true🤖 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 `@pyproject.toml` around lines 152 - 154, Update the [tool.coverage.run] source configuration to measure only the modelconverter package by removing tests from the source list. Preserve relative_files and the existing coverage behavior while ensuring test execution remains recorded.
🤖 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/rvc2/exporter.py`:
- Around line 118-120: Update the color-input handling around the reverse-only
channel conversion so `inp.encoding.from_` and `inp.encoding.to` are changed to
BGR only when that input’s channels were actually reversed. Preserve the
original encoding for unchanged RGB→RGB inputs while retaining the relabeling
for inputs converted from RGB to BGR.
In `@tests/conversion/test_precision.py`:
- Around line 124-130: Update the comparison loop in the precision test to match
converted outputs by key rather than dictionary iteration order: assert that
reference and converted keys match, then retrieve each converted tensor using
ref_name while iterating reference.items(). Preserve the existing output-count
validation and threshold comparison behavior.
In `@tests/conversion/test_toy_integration.py`:
- Around line 218-255: Update the output comparison loop in _assert_correct to
iterate over reference.items() and retrieve each converted tensor from converted
using the corresponding ref_name key, rather than pairing reference and
converted values positionally. Preserve the existing cosine-similarity threshold
assertion and error message.
In `@tests/helpers/onnx_reference.py`:
- Around line 67-94: Update the reference-feed construction around read_image to
pass inp.layout, derive transpose from whether the configured layout ends in C,
and preserve the resulting NHWC/NCHW shape. Use the layout’s channel-axis
position when reshaping mean and scale values, while retaining scalar
broadcasting for inputs without a channel axis.
In `@tests/unit/test_environ.py`:
- Around line 27-67: Add a multiprocessing start-method guard to the affected
tests in tests/unit/test_environ.py, especially test_returns_password and
test_keyring_exception_returns_none, so they skip when the active method is not
fork. Ensure the guard runs before monkeypatch-dependent child-process
assertions, preserving the existing test behavior on fork-based platforms and
avoiding failures under spawn.
---
Outside diff comments:
In `@modelconverter/packages/hailo/exporter.py`:
- Around line 240-256: The HailoExporter._get_alls normalization branch silently
skips configured scale_values or mean_values for non-3-channel inputs. Add a
warning in the values_len != 3 path when either preprocessing setting is
present, identifying the affected input, while preserving the existing behavior
of omitting normalization.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 152-154: Update the [tool.coverage.run] source configuration to
measure only the modelconverter package by removing tests from the source list.
Preserve relative_files and the existing coverage behavior while ensuring test
execution remains recorded.
🪄 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 Plus
Run ID: f6777492-16d6-494d-9e2e-2744035a00c2
📒 Files selected for processing (100)
.dockerignore.github/workflows/ci.yaml.github/workflows/hailo_publish.yaml.github/workflows/hailo_test.yaml.github/workflows/modelconverter_test.yaml.github/workflows/publish.yaml.github/workflows/rvc2_publish.yaml.github/workflows/rvc2_test.yaml.github/workflows/rvc3_publish.yaml.github/workflows/rvc3_test.yaml.github/workflows/rvc4_publish.yaml.github/workflows/rvc4_test.yaml.github/workflows/unittests.yaml.pre-commit-config.yamlREADME.mddocker/hailo/Dockerfiledocker/rvc2/Dockerfiledocker/rvc3/Dockerfiledocker/rvc4/Dockerfilemodelconverter/__init__.pymodelconverter/packages/base_exporter.pymodelconverter/packages/base_inferer.pymodelconverter/packages/getters.pymodelconverter/packages/hailo/exporter.pymodelconverter/packages/hailo/inferer.pymodelconverter/packages/multistage_exporter.pymodelconverter/packages/rvc2/exporter.pymodelconverter/packages/rvc2/inferer.pymodelconverter/packages/rvc3/exporter.pymodelconverter/packages/rvc4/exporter.pymodelconverter/packages/rvc4/inferer.pymodelconverter/utils/calibration_data.pymodelconverter/utils/config.pymodelconverter/utils/docker_utils.pymodelconverter/utils/encodings.pymodelconverter/utils/environ.pymodelconverter/utils/image.pymodelconverter/utils/metadata.pymodelconverter/utils/nn_archive.pymodelconverter/utils/onnx_compatibility.pypyproject.tomlrequirements-dev.txttests/conftest.pytests/conversion/__init__.pytests/conversion/test_conversion.pytests/conversion/test_conversion_variants.pytests/conversion/test_external_data.pytests/conversion/test_hailo_compile.pytests/conversion/test_precision.pytests/conversion/test_rvc4_encodings.pytests/conversion/test_toy_integration.pytests/conversion/test_toy_multistage.pytests/conversion/test_toy_precision.pytests/conversion/test_toy_tflite.pytests/helpers/__init__.pytests/helpers/archive_factory.pytests/helpers/conversion.pytests/helpers/data.pytests/helpers/onnx_factory.pytests/helpers/onnx_reference.pytests/helpers/precision.pytests/helpers/tflite_factory.pytests/test_packages/common.pytests/test_packages/metrics/__init__.pytests/test_packages/metrics/base_metric.pytests/test_packages/metrics/mnist_metric.pytests/test_packages/metrics/resnet_metric.pytests/test_packages/metrics/yolov6_metric.pytests/test_packages/onnx_inferer.pytests/test_packages/test_cross_format_export.pytests/test_packages/test_hailo.pytests/test_packages/test_rvc2.pytests/test_packages/test_rvc3.pytests/test_packages/test_rvc4.pytests/test_utils/conftest.pytests/test_utils/test_config.pytests/test_utils/test_filesystem.pytests/test_utils/test_image.pytests/test_utils/test_layout.pytests/test_utils/test_modifier.pytests/test_utils/test_onnx_compatibility.pytests/test_utils/test_telemetry.pytests/unit/__init__.pytests/unit/conftest.pytests/unit/test_calibration_data.pytests/unit/test_cli_utils.pytests/unit/test_config.pytests/unit/test_config_legacy_yaml.pytests/unit/test_encodings.pytests/unit/test_environ.pytests/unit/test_filesystem_utils.pytests/unit/test_general.pytests/unit/test_getters.pytests/unit/test_image.pytests/unit/test_layout.pytests/unit/test_metadata.pytests/unit/test_nn_archive.pytests/unit/test_random_calibration.pytests/unit/test_target_versions.pytests/unit/test_types.py
💤 Files with no reviewable changes (30)
- .github/workflows/hailo_test.yaml
- .github/workflows/rvc3_publish.yaml
- .github/workflows/rvc2_test.yaml
- .github/workflows/rvc2_publish.yaml
- .github/workflows/rvc3_test.yaml
- tests/test_packages/onnx_inferer.py
- tests/test_packages/metrics/base_metric.py
- tests/test_packages/test_cross_format_export.py
- .github/workflows/unittests.yaml
- tests/test_packages/common.py
- .github/workflows/modelconverter_test.yaml
- .github/workflows/hailo_publish.yaml
- tests/test_utils/conftest.py
- tests/test_packages/metrics/mnist_metric.py
- tests/test_packages/metrics/resnet_metric.py
- tests/test_utils/test_image.py
- tests/test_utils/test_telemetry.py
- .github/workflows/rvc4_publish.yaml
- tests/test_packages/metrics/init.py
- tests/test_utils/test_modifier.py
- .github/workflows/rvc4_test.yaml
- tests/test_packages/test_rvc2.py
- tests/test_utils/test_onnx_compatibility.py
- tests/test_packages/test_rvc3.py
- tests/test_packages/test_hailo.py
- tests/test_packages/test_rvc4.py
- tests/test_packages/metrics/yolov6_metric.py
- tests/test_utils/test_filesystem.py
- tests/test_utils/test_config.py
- tests/test_utils/test_layout.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kozlov721
force-pushed
the
tests/improvement
branch
from
July 27, 2026 09:57
7aee76c to
81b34fe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelconverter/packages/hailo/exporter.py (1)
246-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate normalization vector lengths before emitting the Hailo script.
The config parse path now accepts scalar normalization lists as
[v, v, v], so non-3-channel inputs can still be serialized with legacy 3-element means/scales. Reject mismatchedmean_valuesandscale_valuesbefore appending thenormalization_...command.🤖 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/hailo/exporter.py` around lines 246 - 251, Validate mean_values and scale_values in the normalization emission flow before appending the normalization command: each vector must match values_len, rejecting mismatched lengths, including legacy 3-element values for non-3-channel inputs. Preserve the existing defaults for missing values and only emit normalization after both vectors pass validation.
🧹 Nitpick comments (2)
modelconverter/packages/base_exporter.py (1)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDo not exclude reachable conversion paths from coverage.
Line 133 excludes the entire
simplify_onnxbody, while Lines 204 and 240 hide normal cleanup and explicit validation behavior. Add focused tests/mocks and reservepragma: no coverfor genuinely untestable external failures.Suggested change
- def simplify_onnx(self) -> Path: # pragma: no cover + def simplify_onnx(self) -> Path:Also applies to: 204-204, 240-240
🤖 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/base_exporter.py` at line 133, Remove the `pragma: no cover` exclusions from `simplify_onnx` and the cleanup and validation branches at the referenced locations, then add focused tests with mocks that exercise those reachable conversion paths. Keep coverage exclusions only for genuinely untestable external failure handling.tests/unit/test_evaluation.py (1)
37-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the source-floor failure branch of
assert_quality.Only the max-drop failure path is exercised; the floor-check branch (
reference[metric] < floor) is never triggered by a failing case, leaving that branch uncovered.Suggested addition
+def test_assert_quality_checks_source_floor() -> None: + with pytest.raises(AssertionError, match="below floor"): + assert_quality( + {"AP": 0.10}, + {"AP": 0.10}, + floors={"AP": 0.39}, + max_drops={"AP": 0.03}, + case_id="test", + platform="rvc4", + )🤖 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 `@tests/unit/test_evaluation.py` around lines 37 - 55, Add a failing test case to test_assert_quality_checks_source_floor_and_drop that makes reference[metric] fall below its configured floor while avoiding the max-drop condition, and assert that assert_quality raises AssertionError with the expected floor-related message. Keep the existing max-drop coverage unchanged.
🤖 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/base_exporter.py`:
- Around line 252-255: Update the calibration handling around the is_raw_input
condition so raw inputs retain an array/raw-compatible calibration configuration
when random calibration writes .npy files; do not replace it with
ImageCalibrationConfig unless the data remains image-backed and compatible with
read_img_dir. Preserve the existing image calibration behavior for non-raw
inputs.
In `@tests/conversion/test_model_evaluation.py`:
- Around line 129-172: Update the return type annotation of
_lazy_eval_components to declare all five returned tuple elements, including the
callable corresponding to get_metric_ctx, while preserving the existing parser,
metrics, and mapping callable types.
---
Outside diff comments:
In `@modelconverter/packages/hailo/exporter.py`:
- Around line 246-251: Validate mean_values and scale_values in the
normalization emission flow before appending the normalization command: each
vector must match values_len, rejecting mismatched lengths, including legacy
3-element values for non-3-channel inputs. Preserve the existing defaults for
missing values and only emit normalization after both vectors pass validation.
---
Nitpick comments:
In `@modelconverter/packages/base_exporter.py`:
- Line 133: Remove the `pragma: no cover` exclusions from `simplify_onnx` and
the cleanup and validation branches at the referenced locations, then add
focused tests with mocks that exercise those reachable conversion paths. Keep
coverage exclusions only for genuinely untestable external failure handling.
In `@tests/unit/test_evaluation.py`:
- Around line 37-55: Add a failing test case to
test_assert_quality_checks_source_floor_and_drop that makes reference[metric]
fall below its configured floor while avoiding the max-drop condition, and
assert that assert_quality raises AssertionError with the expected floor-related
message. Keep the existing max-drop coverage unchanged.
🪄 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 Plus
Run ID: 66ee9eb5-289c-462d-bffa-22640ee4478c
📒 Files selected for processing (16)
.github/workflows/ci.yamldocker/rvc2/Dockerfiledocker/rvc4/Dockerfilemodelconverter/packages/base_exporter.pymodelconverter/packages/hailo/exporter.pymodelconverter/packages/rvc2/exporter.pymodelconverter/packages/rvc4/inferer.pyrequirements-eval.txttests/conversion/test_model_evaluation.pytests/conversion/test_precision.pytests/conversion/test_toy_integration.pytests/helpers/evaluation.pytests/helpers/onnx_reference.pytests/unit/test_environ.pytests/unit/test_evaluation.pytests/unit/test_random_calibration.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/test_random_calibration.py
- modelconverter/packages/rvc2/exporter.py
- tests/conversion/test_precision.py
- .github/workflows/ci.yaml
- tests/unit/test_environ.py
- tests/helpers/onnx_reference.py
- docker/rvc4/Dockerfile
- tests/conversion/test_toy_integration.py
kozlov721
force-pushed
the
tests/improvement
branch
from
July 28, 2026 00:42
1854081 to
9224bc2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_benchmark/test_benchmark_regression.py`:
- Around line 215-216: Update test_benchmark_fps to keep the FPS validation
independent of Influx configuration: remove the mandatory influx_token
assertion, allow the conftest-provided optional token, and guard telemetry
persistence so it runs only when influx_token is available while retaining the
FPS check regardless.
- Line 225: Update the expected_fps validation in the benchmark regression test
to accept valid JSON numeric values represented as integers by normalizing
expected_fps with float(...) before comparison, rather than requiring
isinstance(expected_fps, float). Preserve rejection of non-numeric values.
🪄 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 Plus
Run ID: 6d7bbde0-5386-458b-b957-7c9d9ef29afe
📒 Files selected for processing (12)
.github/workflows/ci.yamlmodelconverter/utils/nn_archive.pytests/conversion/test_conversion.pytests/conversion/test_model_evaluation.pytests/helpers/onnx_reference.pytests/test_benchmark/conftest.pytests/test_benchmark/test_benchmark_regression.pytests/unit/test_cli_utils.pytests/unit/test_config.pytests/unit/test_nn_archive.pytests/unit/test_target_versions.pytests/unit/test_types.py
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/unit/test_target_versions.py
- modelconverter/utils/nn_archive.py
- tests/helpers/onnx_reference.py
- .github/workflows/ci.yaml
- tests/conversion/test_conversion.py
- tests/conversion/test_model_evaluation.py
- tests/unit/test_cli_utils.py
- tests/unit/test_config.py
- tests/unit/test_types.py
- tests/unit/test_nn_archive.py
kozlov721
force-pushed
the
tests/improvement
branch
from
July 28, 2026 03:48
29204ed to
f632f62
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Previous tests were insufficient. This PR completely refactors the tests, adding coverage reporting and simplified CI.
Specification
tests/conversiontests for more reliable testing of model conversionsmodelconverter--devflag now also installs thedevrequirements to the image and mountstests/from PWDmodelconverter shell rvc4 --dev -c 'pytest -x'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
layoutmetadata support to image preprocessing and to how conversions/inference interpret inputs.Bug Fixes
Chores