Fix issues in the export recipe framework - #22158
Conversation
None are new; each came from mutating the code and watching the suite stay green. They are separated out so the change that adds a capability can be read on its own. `EdgeProgramManager.transform` deep-copies the graph and state dict of every method its pass dict does not name, and both stages build that dict with a `defaultdict` whose first lazy insertion flips the guard against it. Handing over an empty dict therefore copies methods two onwards to apply nothing — 68 MB on a four-method model with 84 MB of weights, held for the session. Both sites now pass `None`. One visible consequence: with no transform passes, `to_edge_transform_and_lower` no longer records an `edge_after_transform` ETRecord entry, which matches what `devtools/etrecord` already tests for. `_build_stages` hoisted `stage = None` out of its loop, so an unrecognised stage type registered whatever the previous iteration built, and the "register it first" guard could never fire. `_run_pipeline` never cleared `_stage_to_artifacts`, so a failed second `export()` left the accessors mixing runs. It clears after validation, so a pipeline rejected outright leaves the earlier run alone. `ExportRecipe.combine` dropped `edge_compile_config` whenever no recipe contributed a partitioner — the shape of any backend that lowers without delegating, and not optional for them. Carrying it raises the question of two, so configs that disagree are refused rather than resolved by argument order. They compare by value, not identity: every provider builds a fresh object, so `get_ios_recipe` and `get_android_recipe` combine equal configs that must not look like a conflict. `pipeline_stages` is refused outright. `export/tests/test_export_stages.py` is re-enabled. pytorch#12826 pointed `testpaths` at a directory that does not exist, so nothing under `export/tests` had ever been collected in OSS CI; the one file that failed on first collection was ignored rather than repaired. It has been green internally throughout, since `export/tests/BUCK` lists it. Same line removed from `pytest-windows.ini`. Authored with Claude Code.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22158
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 4ae0ab4 with merge base 9a2d135 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
|
@claude please review |
|
Claude finished @JakeStevens's task in 4m 54s —— View job ReviewRead through all eight files. This is a clean, well-reasoned set of fixes — each defect is real, each fix is minimal, and the tests are written to actually fail against the old behavior (not just exercise the new path). The commit message documents the why unusually well. Below are the findings; nothing here is blocking.
Correctness — all five fixes hold up1. Empty pass dict ( 2. 3. Clearing 4. One minor UX gap: 5. Re-enabling Minor / maintenance
SummarySolid, surgical bug-fix PR with tests that genuinely guard each behavior. The only thing I'd actually change is the conflict-error summary (finding 4) so it always shows the fields that differ. Everything else is optional polish. Nice work. |
Summary
EdgeProgramManager.transformdeep-copies the graph and state dict of every method its pass dict does not name, and both stages build that dict with adefaultdictwhose first lazy insertion flips the guard against it. Handing over an empty dict therefore copies methods two onwards to apply nothing — 68 MB on a four-method model with 84 MB of weights, held for the session. Both sites now passNone. One visible consequence: with no transform passes,to_edge_transform_and_lowerno longer records anedge_after_transformETRecord entry, which matches whatdevtools/etrecordalready tests for._build_stageshoistedstage = Noneout of its loop, so an unrecognised stage type registered whatever the previous iteration built, and the "register it first" guard could never fire._run_pipelinenever cleared_stage_to_artifacts, so a failed secondexport()left the accessors mixing runs. It clears after validation, so a pipeline rejected outright leaves the earlier run alone.ExportRecipe.combinedroppededge_compile_configwhenever no recipe contributed a partitioner — the shape of any backend that lowers without delegating, and not optional for them. Carrying it raises the question of two, so configs that disagree are refused rather than resolved by argument order. They compare by value, not identity: every provider builds a fresh object, soget_ios_recipeandget_android_recipecombine equal configs that must not look like a conflict.pipeline_stagesis refused outright.export/tests/test_export_stages.pyis re-enabled. [ET] Fix pytest path for executorch export tests #12826 pointedtestpathsat a directory that does not exist, so nothing underexport/testshad ever been collected in OSS CI; the one file that failed on first collection was ignored rather than repaired. It has been green internally throughout, sinceexport/tests/BUCKlists it. Same line removed frompytest-windows.ini.Authored with Claude Code.