feat: include variant name in ONNX export filename#910
feat: include variant name in ONNX export filename#910farukalamai wants to merge 2 commits intoroboflow:developfrom
Conversation
farukalamai
requested review from
Borda,
SkalskiP,
isaacrob and
probicheaux
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (78%) is below the target coverage (95%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #910 +/- ##
======================================
Coverage 78% 78%
======================================
Files 97 97
Lines 7706 7708 +2
======================================
+ Hits 6014 6016 +2
Misses 1692 1692 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the ONNX export pipeline to incorporate the model variant name into the exported filename (e.g. rfdetr-medium.onnx), preventing accidental overwrites when exporting multiple variants to the same directory.
Changes:
- Add optional
variant_nameparameter to ONNX exporter and use it to derive the output filename (with-backbonesuffix for backbone-only exports). - Pass the model’s variant identifier (
self.size) throughRFDETR.export()into the ONNX exporter. - Add tests covering variant-based naming, backbone suffix behavior, fallback naming, and
RFDETR.export()integration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/rfdetr/export/_onnx/exporter.py |
Introduces variant_name and uses it to build the ONNX output filename. |
src/rfdetr/detr.py |
Forwards self.size into export_onnx() so high-level RFDETR.export() produces variant-specific filenames. |
src/rfdetr/export/main.py |
Threads an (optional) args.variant_name through the CLI export path. |
tests/models/test_export.py |
Adds coverage ensuring naming behavior works for variant/backbone/fallback and RFDETR integration. |
| if variant_name: | ||
| export_name = f"{variant_name}-backbone" if backbone_only else variant_name | ||
| else: | ||
| export_name = "backbone_model" if backbone_only else "inference_model" | ||
| output_file = os.path.join(output_dir, f"{export_name}.onnx") | ||
|
|
There was a problem hiding this comment.
variant_name is interpolated directly into the filename. If it contains path separators (e.g. "foo/bar"), an absolute path (e.g. "/tmp/x"), or a drive prefix on Windows, os.path.join(output_dir, ...) can write outside output_dir or fail due to missing intermediate directories. Consider sanitizing to a safe stem (e.g. Path(variant_name).name/.stem, stripping a trailing .onnx) and rejecting values that aren’t simple filenames.
| When provided, the exported file is named ``{variant_name}.onnx`` | ||
| instead of the generic ``inference_model.onnx``. |
There was a problem hiding this comment.
The variant_name docstring says the export will be named {variant_name}.onnx, but when backbone_only=True the actual filename becomes {variant_name}-backbone.onnx. Update the docstring to reflect the backbone-only naming behavior so callers aren’t surprised.
| When provided, the exported file is named ``{variant_name}.onnx`` | |
| instead of the generic ``inference_model.onnx``. | |
| When provided, the exported file is named ``{variant_name}.onnx`` or | |
| ``{variant_name}-backbone.onnx`` (when ``backbone_only=True``) instead | |
| of the generic ``inference_model.onnx`` or ``backbone_model.onnx``. |
| output_file = export_onnx( | ||
| output_dir=str(output_dir_path), | ||
| model=model, | ||
| input_names=input_names, | ||
| input_tensors=input_tensors, | ||
| output_names=output_names, | ||
| dynamic_axes=dynamic_axes, | ||
| backbone_only=backbone_only, | ||
| verbose=verbose, | ||
| opset_version=opset_version, | ||
| variant_name=getattr(self, "size", None), | ||
| ) |
There was a problem hiding this comment.
RFDETR.export() still accepts **kwargs and the docstring says they’re forwarded to export_onnx, but the export_onnx(...) call doesn’t actually pass **kwargs. With the new variant_name= argument being set explicitly, callers also can’t override it via kwargs. Either forward **kwargs into export_onnx (and decide precedence), or remove/update the **kwargs parameter/docstring to match behavior.
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # ONNX export variant naming (#issue) |
There was a problem hiding this comment.
The section header comment says # ONNX export variant naming (#issue) which looks like a placeholder. Replace it with the actual issue reference (e.g. #905) or remove the parenthetical to avoid stale/incorrect references in the test file.
| # ONNX export variant naming (#issue) | |
| # ONNX export variant naming |
What does this PR do?
ONNX export now names the output file after the model variant (e.g.
rfdetr-medium.onnx) instead of the genericinference_model.onnx. Exporting multiple variants to the same directory no longer overwrites previous exports.Related Issue(s): Part of #905
Type of Change
Testing
Test details:
Added
TestExportOnnxVariantNamingwith 6 tests covering variant naming, backbone suffix, default fallback, andRFDETR.export()integration.Checklist