fix(export): make export-mode switch idempotent - #1445
Conversation
- `_switch_to_export_mode` now returns early when the module reports `_export` is already true, so a repeated switch cannot damage the model - Record why in the helper's docstring: `LWDETR.export`, `Backbone.export` and `PositionEmbeddingSine.export` each stash `self._forward_origin = self.forward` before swapping in `forward_export`, so a second call overwrites the saved original with the export forward and loses the real one for good; `DinoV2.export` already guards itself, these three do not - Add `TestSwitchToExportMode` covering the three cases: a fresh model is switched once, a second call is a no-op, and a plain `nn.Module` without an `export` attribute passes through untouched - No behaviour change on any current call path: every caller receives a freshly deepcopied module, and backbone-only exports pass a `_BackboneExport` wrapper that exposes no `export` attribute, so the helper already no-opped there --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The ONNX export path still bypasses the new guard, leaving the reported corruption scenario unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Makes the shared export-mode switch idempotent by skipping models already in export mode.
Changes:
- Adds an export-state guard and supporting rationale.
- Adds focused tests for initial, repeated, and unsupported switches.
File summaries
| File | Review |
|---|---|
src/rfdetr/export/_backend.py |
Guard does not cover ONNX’s direct export() call, so composed exports can still corrupt _forward_origin. |
tests/export/test_export.py |
Tests helper idempotence but lacks regression coverage for composed ONNX export preparation. |
Review details
Suppressed comments (1)
tests/export/test_export.py:1474
- This test only invokes
_switch_to_export_modetwice, so it does not exercise the exporter-composition scenario described here. A composed ONNX export bypasses this helper and callsmodel.export()directly, meaning this test passes while that scenario still corrupts_forward_origin. Add a regression that performs ONNX preparation after the first guarded switch (mocking the actual ONNX serialization) so all switching entry points are covered.
The failure this guards is silent and unrecoverable: the second ``export()`` would overwrite
``_forward_origin`` with ``forward_export``, so the module could never be restored. Two
switches become reachable as soon as one exporter composes another (a TFLite or TensorRT
export running an ONNX export internally).
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (``DinoV2.export`` already guards itself; these three do not.) The guard lives here rather than in | ||
| the models so every export path shares one choke point. |
There was a problem hiding this comment.
Fixed in 4e8bd9d: export_onnx now routes its export-mode switch through _switch_to_export_mode (the guarded choke point in _backend.py) instead of calling model.export() directly, so a composed ONNX export after a prior switch no longer invokes export() twice or overwrites _forward_origin. Also refreshed the helper's docstring (it is no longer "non-ONNX only") and added regression tests pinning both the composed switch-then-ONNX path and the fresh-model ONNX path to exactly one switch (TestSwitchToExportMode::test_export_onnx_routes_through_the_guarded_switch / test_export_onnx_still_switches_a_fresh_model; the former fails with switches == 2 when the fix is reverted).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1445 +/- ##
=======================================
Coverage 87% 87%
=======================================
Files 117 117
Lines 15260 15262 +2
=======================================
+ Hits 13214 13216 +2
Misses 2046 2046 🚀 New features to boost your workflow:
|
`export_onnx` called `model.export()` directly, bypassing the `_export`-guard in `_switch_to_export_mode` — so a composed export (e.g. a prior switch followed by an ONNX export) invoked `export()` twice and overwrote `_forward_origin`, the exact failure the guard was added to prevent. Route ONNX preparation through the same guarded choke point, refresh the now-stale docstring/comment claims, and add regression tests pinning both the composed (switch-then-ONNX) and fresh-model ONNX paths to exactly one switch. Co-authored-by: Borda <6035284+Borda@users.noreply.github.com>
_switch_to_export_modenow returns early when the module reports_exportis already true, so a repeated switch cannot damage the modelLWDETR.export,Backbone.exportandPositionEmbeddingSine.exporteach stashself._forward_origin = self.forwardbefore swapping inforward_export, so a second call overwrites the saved original with the export forward and loses the real one for good;DinoV2.exportalready guards itself, these three do notTestSwitchToExportModecovering the three cases: a fresh model is switched once, a second call is a no-op, and a plainnn.Modulewithout anexportattribute passes through untouched_BackboneExportwrapper that exposes noexportattribute, so the helper already no-opped there