Ensure that the ONNX IR version is the max supported version (10) #416
Conversation
WalkthroughAdds MAX_IR_VERSION = 10 and updates load_onnx_model to clamp an ONNX model's ir_version to 10 when higher, save a suffixed copy (preferring static-shaped path), record it in intermediate_generated_files, validate with onnx.checker, and prefer that path on return. Adds a unit test asserting the clamped ir_version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Loader as load_onnx_model
participant Checker as onnx.checker
participant FS as Filesystem
Caller->>Loader: load_onnx_model(onnx_path, static_shaped_onnx_path?, ...)
Loader->>Loader: load model (and infer shapes/types)
Loader->>Loader: read onnx_model.ir_version
alt ir_version > MAX_IR_VERSION (10)
Loader->>Loader: set onnx_model.ir_version = 10
Loader->>Checker: onnx.checker.check_model(onnx_model)
Loader->>FS: save model to <base>_ir10.onnx (prefer static_shaped path)
Loader->>Loader: append path to intermediate_generated_files (if given)
Loader-->>Caller: return path to *_ir10.onnx
else
Loader-->>Caller: return static_shaped_onnx_path or original onnx_path
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
ec3fe85 to
321bf18
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
=======================================
Coverage 73.42% 73.42%
=======================================
Files 180 180
Lines 17965 17975 +10
=======================================
+ Hits 13191 13199 +8
- Misses 4774 4776 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
377-431: LGTM with a minor suggestion.The function correctly builds a MatMul→Relu model with IR version 12 for testing. The implementation is sound.
Consider updating the graph name from
"r1a"(line 420) to something more descriptive like"matmul_relu"to better reflect the model structure.- graph = helper.make_graph(nodes, "r1a", inputs, outputs, initializer=initializers) + graph = helper.make_graph(nodes, "matmul_relu", inputs, outputs, initializer=initializers)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modelopt/onnx/trt_utils.py(2 hunks)tests/_test_utils/onnx_quantization/lib_test_models.py(1 hunks)tests/unit/onnx/test_onnx_utils.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/onnx/test_onnx_utils.py (2)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
build_matmul_relu_model_ir_12(377-431)modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-341)
modelopt/onnx/trt_utils.py (1)
modelopt/onnx/utils.py (1)
save_onnx(642-679)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
modelopt/onnx/utils.py (1)
check_model(557-569)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: multi-py (11)
- GitHub Check: multi-py (10)
- GitHub Check: multi-torch (27)
- GitHub Check: windows
- GitHub Check: partial-install (onnx)
- GitHub Check: multi-transformers (min)
- GitHub Check: multi-torch (26)
- GitHub Check: partial-install (torch)
- GitHub Check: wait-checks / wait
🔇 Additional comments (5)
tests/unit/onnx/test_onnx_utils.py (2)
21-21: LGTM!The import of
build_matmul_relu_model_ir_12is correctly added to support the new IR version test.
32-32: LGTM!The import of
load_onnx_modelis correctly added to support the IR version clamping test.modelopt/onnx/trt_utils.py (3)
39-40: LGTM!The
MAX_IR_VERSIONconstant is well-defined and clearly indicates the maximum supported IR version. This makes the clamping logic more maintainable.
323-334: LGTM!The IR version clamping logic correctly handles models with IR version > 10 by:
- Clamping the version to
MAX_IR_VERSION- Generating an appropriate intermediate file path
- Saving the modified model
- Tracking the intermediate file for cleanup
The logic properly handles both
static_shaped_onnx_pathandonnx_pathcases.Note: The
save_onnxfunction (inmodelopt/onnx/utils.pyline 662) also setsir_version = 10, creating slight redundancy. However, this is acceptable since the in-memory model returned byload_onnx_modelneeds the clamped version, and the explicit assignment here makes the intent clear.
335-341: LGTM!The return statement correctly prioritizes
ir_version_onnx_pathwhen IR clamping occurs, ensuring the clamped model path is returned. The fallback order (ir_version_onnx_path → static_shaped_onnx_path → onnx_path) aligns with the processing pipeline.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
bc4d58c to
3f02fec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
377-431: Add a docstring for consistency.The function implementation is correct and serves its purpose of creating an IR version 12 test model. However, other model builders in this file include docstrings (e.g.,
build_resnet_block()at line 200), so adding one here would improve consistency.Apply this diff to add a docstring:
def build_matmul_relu_model_ir_12(): + """Build a simple MatMul->Relu model with IR version 12 for testing IR version clamping.""" # Define your model inputs and outputs input_names = ["input_0"]Note: The AI summary mentions this function is duplicated in the file, but I only see one definition in the provided code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modelopt/onnx/trt_utils.py(2 hunks)tests/_test_utils/onnx_quantization/lib_test_models.py(1 hunks)tests/unit/onnx/test_onnx_utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/onnx/test_onnx_utils.py
- modelopt/onnx/trt_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
modelopt/onnx/utils.py (1)
check_model(557-569)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
410-416: Align initializer dtype with TensorProto.FLOAT
np.random.uniformyieldsfloat64, so we rely on implicit casts when serializing. Casting once tofloat32keeps the helper aligned with the declared tensor type and avoids unnecessary conversions.helper.make_tensor( name="weights_1", data_type=onnx.TensorProto.FLOAT, dims=(1024, 16), - vals=np.random.uniform(low=0.5, high=1.0, size=1024 * 16), + vals=np.random.uniform(low=0.5, high=1.0, size=1024 * 16).astype(np.float32), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/_test_utils/onnx_quantization/lib_test_models.py(1 hunks)tests/unit/onnx/test_onnx_utils.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/onnx/test_onnx_utils.py (2)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
build_matmul_relu_model(377-433)modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-341)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
modelopt/onnx/utils.py (1)
check_model(557-569)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (1)
tests/unit/onnx/test_onnx_utils.py (1)
260-267: IR clamp regression test looks goodThanks for pinning the regression with a high-IR model and exercising the clamp path. This keeps the new guard from regressing.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
e400d7f to
cf8dc63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modelopt/onnx/trt_utils.py (2)
323-333: Consider consolidating file generation to avoid redundant saves.When both static shape overriding and IR version clamping are needed, the current logic creates two intermediate files:
static_shaped_onnx_path(line 300) with static shapesir_version_onnx_path(line 332) with both static shapes and clamped IRThis is redundant since the IR-clamped file already contains the static shapes. Consider refactoring to:
- Check and clamp IR version first (before shape inference)
- Then apply static shapes if needed
- Save only once with both transformations applied
This would reduce disk I/O and avoid confusion with multiple intermediate files.
Additionally, note that
save_onnx(modelopt/onnx/utils.py:661) already setsir_version = 10, so any subsequent saves will clamp the IR version anyway. Ensure the clamping at load time serves a clear purpose beyond what's already done at save time.Based on relevant code snippets.
327-331: Path naming could be clearer when both transformations apply.When both static shapes and IR clamping are applied, the resulting path becomes something like
model_static_ir10.onnx, which may be confusing. Consider using a more explicit naming convention that clearly indicates both transformations, such as:
model_static_ir10.onnx→model_transformed.onnxor- Use a timestamp/hash to avoid conflicts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modelopt/onnx/trt_utils.py(2 hunks)tests/_test_utils/onnx_quantization/lib_test_models.py(1 hunks)tests/unit/onnx/test_onnx_utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/onnx/test_onnx_utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/_test_utils/onnx_quantization/lib_test_models.py (1)
modelopt/onnx/utils.py (1)
check_model(557-569)
modelopt/onnx/trt_utils.py (1)
modelopt/onnx/utils.py (1)
save_onnx(642-679)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (4)
modelopt/onnx/trt_utils.py (2)
39-40: LGTM! Clear constant definition.Defining
MAX_IR_VERSION = 10as a module-level constant improves maintainability and makes it easy to update in the future.
339-339: LGTM! Return path selection is correct.The updated return statement correctly prioritizes
ir_version_onnx_pathwhen IR clamping occurs, falling back tostatic_shaped_onnx_pathor the originalonnx_path.tests/_test_utils/onnx_quantization/lib_test_models.py (2)
377-433: Function implementation looks correct.The
build_matmul_relu_modelfunction properly constructs an ONNX model with:
- MatMul followed by ReLU operations
- Appropriate input/output shapes
- Weight initializer
- Configurable IR version (defaulting to 12 for testing)
This provides a good test case for verifying IR version clamping behavior.
377-433: No duplicate function definition. Only onebuild_matmul_relu_modelexists (line 377).Likely an incorrect or invalid review comment.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
b01bc7a to
d3e27a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
modelopt/onnx/trt_utils.py (2)
326-326: Add model validation after IR version modification.As suggested in a previous review, add
onnx.checker.check_model()after modifying the IR version to ensure the change doesn't break the model.Apply this diff:
if onnx_model.ir_version > MAX_IR_VERSION: onnx_model.ir_version = MAX_IR_VERSION + onnx.checker.check_model(onnx_model) ir_version_onnx_path = (
333-333: Check intermediate_generated_files for None before appending.The
# type: ignore[union-attr]comment suppresses a type error becauseintermediate_generated_filescan beNone(per the type hint at line 250). Add an explicit check instead of suppressing the type error.Apply this diff:
save_onnx(onnx_model, ir_version_onnx_path, use_external_data_format) - intermediate_generated_files.append(ir_version_onnx_path) # type: ignore[union-attr] + if intermediate_generated_files is not None: + intermediate_generated_files.append(ir_version_onnx_path)Note: The same fix should be applied at line 301 for consistency (already flagged in past review comments).
🧹 Nitpick comments (4)
modelopt/onnx/trt_utils.py (1)
323-333: Redundant IR version assignment in save_onnx.The code clamps
onnx_model.ir_versiontoMAX_IR_VERSIONat line 326, then callssave_onnxat line 332. However,save_onnx(modelopt/onnx/utils.py, line 662) also unconditionally setsmodel.ir_version = 10. This creates redundant IR version assignment.While not causing incorrect behavior, this redundancy could be confusing. Consider either:
- Documenting that
save_onnxwill handle the IR version clamping, or- Removing the clamping in
save_onnxsince it's now handled heretests/unit/onnx/test_onnx_utils.py (3)
259-315: Consider using MAX_IR_VERSION constant instead of magic number.The default
ir_version=12is a magic number. Consider importingMAX_IR_VERSIONfrommodelopt.onnx.trt_utilsand using a value relative to it (e.g.,MAX_IR_VERSION + 2) to make the test more maintainable and self-documenting.Apply this diff:
+from modelopt.onnx.trt_utils import MAX_IR_VERSION, load_onnx_model -from modelopt.onnx.trt_utils import load_onnx_model -def _make_matmul_relu_model(ir_version=12): +def _make_matmul_relu_model(ir_version=MAX_IR_VERSION + 2):
297-297: Random weights may cause non-deterministic test behavior.The helper uses
np.random.uniform()without setting a seed, which could lead to non-deterministic test behavior. While this test only checks IR version (not weight values), setting a seed ensures reproducibility.Apply this diff before calling
np.random.uniform:+ np.random.seed(42) initializers = [ make_tensor( name="weights_1",
318-325: Test validates IR version clamping correctly.The test effectively validates that models with IR version > 10 are clamped to 10 when loaded. The assertion message is clear and helpful.
Optional: Consider more comprehensive validation.
The test could additionally verify:
- The intermediate file path was created with the expected
_ir10.onnxsuffix- The path was recorded in the
intermediate_generated_fileslist- The returned path (4th tuple element) points to the IR-clamped file
However, the current test provides sufficient coverage for the core clamping behavior.
Example enhancement:
def test_ir_version_support(tmp_path): model = _make_matmul_relu_model(ir_version=12) model_path = os.path.join(tmp_path, "test_matmul_relu.onnx") onnx.save(model, model_path) intermediate_files = [] model_reload, _, _, returned_path, _ = load_onnx_model( model_path, intermediate_generated_files=intermediate_files ) assert model_reload.ir_version == 10, ( f"The maximum supported IR version is 10, but version {model_reload.ir_version} was detected." ) assert returned_path.endswith("_ir10.onnx"), "Returned path should reference IR-clamped file" assert len(intermediate_files) == 1, "Should have recorded the intermediate file" assert intermediate_files[0] == returned_path, "Intermediate file should match returned path"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modelopt/onnx/trt_utils.py(2 hunks)tests/unit/onnx/test_onnx_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/onnx/test_onnx_utils.py (1)
modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-341)
modelopt/onnx/trt_utils.py (1)
modelopt/onnx/utils.py (1)
save_onnx(642-679)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (3)
modelopt/onnx/trt_utils.py (2)
39-39: LGTM: Well-defined constant.The module-level constant provides a single source of truth for the maximum supported IR version, improving maintainability.
339-339: LGTM: Correct path preference order.The return statement correctly prioritizes
ir_version_onnx_pathwhen IR version clamping occurred, falling back tostatic_shaped_onnx_pathor the originalonnx_path.tests/unit/onnx/test_onnx_utils.py (1)
31-31: LGTM: Necessary import for the new test.The import of
load_onnx_modelis correctly placed and required for the IR version test.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
333eb4d to
81124be
Compare
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
81124be to
f4d3047
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modelopt/onnx/trt_utils.py (1)
323-334: Critical: Fix the AttributeError whenintermediate_generated_filesis None.Lines 333 and 301 append to
intermediate_generated_fileswithout checking if it'sNone, even though the type hint indicates it can beNone(with a default value ofNone). This will cause anAttributeErrorwhen callers don't provide this parameter.This issue was flagged in a previous review but remains unaddressed.
Apply this diff to fix both locations:
if override_shapes: # ... existing code ... static_shaped_onnx_path = onnx_path.replace(".onnx", "_static.onnx") save_onnx(onnx_model, static_shaped_onnx_path, use_external_data_format) - intermediate_generated_files.append(static_shaped_onnx_path) # type: ignore[union-attr] + if intermediate_generated_files is not None: + intermediate_generated_files.append(static_shaped_onnx_path) # ... existing code ... # Enforce IR version = 10 ir_version_onnx_path = None if onnx_model.ir_version > MAX_IR_VERSION: onnx_model.ir_version = MAX_IR_VERSION ir_version_onnx_path = ( static_shaped_onnx_path.replace(".onnx", f"_ir{MAX_IR_VERSION}.onnx") if static_shaped_onnx_path else onnx_path.replace(".onnx", f"_ir{MAX_IR_VERSION}.onnx") ) save_onnx(onnx_model, ir_version_onnx_path, use_external_data_format) - intermediate_generated_files.append(ir_version_onnx_path) # type: ignore[union-attr] + if intermediate_generated_files is not None: + intermediate_generated_files.append(ir_version_onnx_path)
🧹 Nitpick comments (2)
tests/unit/onnx/test_onnx_utils.py (2)
259-315: Helper function is well-structured.The
_make_matmul_relu_modelhelper creates a simple ONNX model with a configurable IR version, which is appropriate for testing the IR version clamping behavior. The function properly creates the graph, sets the IR version, and validates the model.Optional: Consider setting a random seed for reproducibility.
While not critical for this test (since only the IR version is being checked), setting a random seed before
np.random.uniformat line 297 would make the test more deterministic.
318-325: Test correctly validates IR version clamping.The test properly verifies that models with IR version 12 are clamped to IR version 10 when loaded through
load_onnx_model. The assertion message is clear and informative.Consider verifying the intermediate file tracking.
The test passes an empty list for
intermediate_generated_files=[]but doesn't verify that the intermediate file was actually created and tracked. Consider adding assertions to check:intermediate_files = [] model_reload, _, _, returned_path, _ = load_onnx_model(model_path, intermediate_generated_files=intermediate_files) assert model_reload.ir_version == 10 assert len(intermediate_files) == 1 # Verify intermediate file was tracked assert "ir10" in returned_path # Verify returned path is the IR-clamped version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modelopt/onnx/trt_utils.py(2 hunks)tests/unit/onnx/test_onnx_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/onnx/test_onnx_utils.py (1)
modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-344)
modelopt/onnx/trt_utils.py (1)
modelopt/onnx/utils.py (2)
save_onnx(642-679)check_model(557-569)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (4)
tests/unit/onnx/test_onnx_utils.py (1)
31-31: LGTM: Import statement is correct.The import of
load_onnx_modelis necessary for the new test.modelopt/onnx/trt_utils.py (3)
39-39: LGTM: Constant definition is clear.Defining
MAX_IR_VERSION = 10as a module-level constant is good practice and makes the IR version cap explicit and easy to update.
335-336: LGTM: Model validation after IR version change.Calling
onnx.checker.check_modelafter modifying the IR version ensures the model remains valid. This addresses the previous review feedback appropriately.
342-342: LGTM: Return statement correctly prioritizes the IR-clamped path.The return statement properly returns
ir_version_onnx_pathwhen the IR version was clamped, falling back tostatic_shaped_onnx_pathor the originalonnx_pathotherwise.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
f4d3047 to
88d9df5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/onnx/test_onnx_utils.py (2)
259-316: Consider setting a random seed for reproducibility.The helper function correctly creates an ONNX model for testing IR version clamping. However, the use of
np.random.uniformon line 297 without a fixed seed could affect test reproducibility, though the impact is minimal since the test only verifies IR version clamping, not computational results.If desired, add a seed before line 297:
+ np.random.seed(42) initializers = [ make_tensor( name="weights_1",
318-325: Consider verifying intermediate file creation for test completeness.The test correctly verifies that the IR version is clamped to 10, but it could be more thorough by also checking that the intermediate file with the clamped IR version was created and tracked.
Apply this diff to enhance test coverage:
def test_ir_version_support(tmp_path): model = _make_matmul_relu_model(ir_version=12) model_path = os.path.join(tmp_path, "test_matmul_relu.onnx") onnx.save(model, model_path) - model_reload, _, _, _, _ = load_onnx_model(model_path, intermediate_generated_files=[]) + intermediate_files = [] + model_reload, _, _, returned_path, _ = load_onnx_model( + model_path, intermediate_generated_files=intermediate_files + ) assert model_reload.ir_version == 10, ( f"The maximum supported IR version is 10, but version {model_reload.ir_version} was detected." ) + + # Verify intermediate file was created with correct suffix + assert len(intermediate_files) == 1, "Expected one intermediate file to be generated" + assert intermediate_files[0].endswith("_ir10.onnx"), "Intermediate file should have _ir10.onnx suffix" + assert os.path.exists(intermediate_files[0]), "Intermediate file should exist" + assert returned_path == intermediate_files[0], "Returned path should point to the IR-clamped file"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modelopt/onnx/trt_utils.py(2 hunks)tests/unit/onnx/test_onnx_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/onnx/trt_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/onnx/test_onnx_utils.py (1)
modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-344)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (1)
tests/unit/onnx/test_onnx_utils.py (1)
31-31: LGTM!The import is correctly placed and necessary for testing the IR version clamping functionality.
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
88d9df5 to
5e3ee4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/onnx/test_onnx_utils.py (2)
259-315: Consider setting a random seed for reproducibility.The random weights at line 297 make the model non-deterministic. While this doesn't affect the IR version test, setting a seed improves test reproducibility.
Apply this diff to set a random seed:
def _make_matmul_relu_model(ir_version=12): + np.random.seed(42) # Define your model inputs and outputs input_names = ["input_0"]
318-325: Consider expanding test coverage to verify intermediate file handling.The test validates that the IR version is clamped to 10, but doesn't verify that the intermediate file was created or that the returned path points to the IR-versioned model.
Apply this diff to strengthen the test:
def test_ir_version_support(tmp_path): model = _make_matmul_relu_model(ir_version=12) model_path = os.path.join(tmp_path, "test_matmul_relu.onnx") onnx.save(model, model_path) - model_reload, _, _, _, _ = load_onnx_model(model_path, intermediate_generated_files=[]) + intermediate_files = [] + model_reload, _, _, returned_path, _ = load_onnx_model(model_path, intermediate_generated_files=intermediate_files) assert model_reload.ir_version == 10, ( f"The maximum supported IR version is 10, but version {model_reload.ir_version} was detected." ) + # Verify intermediate file was created and tracked + assert len(intermediate_files) == 1, "Expected one intermediate file to be created" + assert os.path.exists(intermediate_files[0]), "Intermediate file should exist on disk" + assert "_ir10.onnx" in returned_path, "Returned path should point to IR-versioned model"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modelopt/onnx/trt_utils.py(2 hunks)tests/unit/onnx/test_onnx_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/onnx/trt_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/onnx/test_onnx_utils.py (1)
modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-344)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (1)
tests/unit/onnx/test_onnx_utils.py (1)
31-31: LGTM!The import is necessary for the new test and properly placed.
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
Signed-off-by: gcunhase <[email protected]>
gcunhase
force-pushed
the
dev/gcunhasergio/fix_max_ir_version_10
branch
from
0f40fe9 to
b57d6cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/onnx/test_onnx_utils.py (2)
259-315: Consider using fixed values or seeded random for test determinism.The helper uses
np.random.uniform(line 297) without a seed, which could introduce non-determinism in test execution. While the current test only validates IR version and doesn't depend on weight values, using fixed values or a seeded RNG improves test stability and reproducibility.Apply this diff if you prefer fixed values:
# Create the ONNX initializers initializers = [ make_tensor( name="weights_1", data_type=onnx.TensorProto.FLOAT, dims=(1024, 16), - vals=np.random.uniform(low=0.5, high=1.0, size=1024 * 16), + vals=np.ones(1024 * 16, dtype=np.float32), ), ]Or seed the RNG at the start of the function:
def _make_matmul_relu_model(ir_version=12): + np.random.seed(42) # Define your model inputs and outputs
318-325: Expand test coverage to verify file generation and path tracking.The test correctly validates IR version capping but misses verification of related functionality:
- The
intermediate_generated_fileslist should contain the newly created IR-capped model path.- The returned path (4th return value) should be the
ir_version_onnx_pathwhen IR version is modified.- Consider adding test cases for IR version ≤ 10 to ensure no unnecessary modifications occur.
Apply this diff to enhance the test:
def test_ir_version_support(tmp_path): + intermediate_files = [] model = _make_matmul_relu_model(ir_version=12) model_path = os.path.join(tmp_path, "test_matmul_relu.onnx") onnx.save(model, model_path) - model_reload, _, _, _, _ = load_onnx_model(model_path, intermediate_generated_files=[]) + model_reload, _, _, returned_path, _ = load_onnx_model( + model_path, intermediate_generated_files=intermediate_files + ) assert model_reload.ir_version == 10, ( f"The maximum supported IR version is 10, but version {model_reload.ir_version} was detected." ) + assert len(intermediate_files) == 1, "Expected one intermediate file to be generated" + assert returned_path == intermediate_files[0], "Returned path should be the IR-capped model" + assert os.path.exists(returned_path), "IR-capped model file should exist"Additionally, consider adding a test case for models with IR version ≤ 10:
def test_ir_version_no_modification(tmp_path): """Verify models with IR version ≤ 10 are not modified.""" intermediate_files = [] model = _make_matmul_relu_model(ir_version=9) model_path = os.path.join(tmp_path, "test_matmul_relu_v9.onnx") onnx.save(model, model_path) model_reload, _, _, returned_path, _ = load_onnx_model( model_path, intermediate_generated_files=intermediate_files ) assert model_reload.ir_version == 9 assert len(intermediate_files) == 0, "No intermediate files should be generated" assert returned_path == model_path, "Original path should be returned"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modelopt/onnx/trt_utils.py(3 hunks)tests/unit/onnx/test_onnx_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modelopt/onnx/trt_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/onnx/test_onnx_utils.py (1)
modelopt/onnx/trt_utils.py (1)
load_onnx_model(245-346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (1)
tests/unit/onnx/test_onnx_utils.py (1)
31-31: LGTM!The import is correctly added to support the new IR version test.
2 participants
What does this PR do?
Type of change: Bug fix
Overview: This PR ensures that ONNX models saved with IR version higher than 10 are still supported in ModelOpt.
Usage
Testing
Added unittest.
Before your PR is "Ready for review"
Summary by CodeRabbit
Bug Fixes
Tests