Skip to content

Fix issues in the export recipe framework - #22158

Open
rascani wants to merge 1 commit into
pytorch:mainfrom
rascani:export-recipe-bugfixes
Open

Fix issues in the export recipe framework#22158
rascani wants to merge 1 commit into
pytorch:mainfrom
rascani:export-recipe-bugfixes

Conversation

@rascani

@rascani rascani commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  1. 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.
  2. _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.
  3. _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.
  4. 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.
  5. export/tests/test_export_stages.py is re-enabled. [ET] Fix pytest path for executorch export tests #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.

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.
@pytorch-bot

pytorch-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

🔗 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 (image):

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.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 25, 2026
@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@rascani
rascani requested a review from JakeStevens August 25, 2026 22:49
@rascani
rascani marked this pull request as ready for review August 25, 2026 22:49
@JakeStevens

Copy link
Copy Markdown
Contributor

@claude please review

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Claude finished @JakeStevens's task in 4m 54s —— View job


Review

Read 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.

  • Read the changed source files
  • Analyze each of the five fixes
  • Review the test changes
  • Post review feedback

Correctness — all five fixes hold up

1. Empty pass dict (stages.py)final_passes = pass_manager or _drop_empty(transform_passes) or None correctly collapses an all-empty defaultdict to None, and the EdgeProgramManagerTransformStage guard (if final_passes is not None) skips the needless transform() entirely. Good catch on the defaultdict interaction — the lazy insertion in the resolve loop is exactly what flips an "empty" dict into a truthy one. Note that a partial dict (method A has passes, method B resolves to none) still drops B and lets transform() deep-copy B to apply nothing — but that's inherent to the EdgeProgramManager.transform API, not something this fix can avoid. Correct as written.

2. stage = None hoist (export.py:301) — moving the reset inside the loop is the right fix; the is not None guard is also more correct than the old truthiness check (a stage object is always truthy, but this reads clearly). ✅

3. Clearing _stage_to_artifacts (export.py:446) — clearing after _validate_pipeline_sequence is the important detail, and both the "re-run discards" and "rejected pipeline preserved" cases are tested. The in-place .clear() (vs. reassigning = {}) is a deliberate, documented, and tested contract because get_stage_artifacts() hands out the live dict. Worth flagging only because it's mildly surprising API behavior — a caller who captured the dict from a prior run sees it emptied under them — but it's now pinned by a test, so intentional.

4. combine edge_compile_config (recipe.py) — the value-based conflict detection is the right call, and deep-copying distinct[0][1] before handing it to the combined recipe (tested via the preserve_ops.append mutation check) prevents aliasing back into a provider's config. The frozenset treatment of preserve_ops / _core_aten_ops_exception_list correctly makes ordering irrelevant.

One minor UX gap: _edge_compile_config_summary only surfaces _check_ir_validity and preserve_ops. If two configs disagree only on _skip_dim_order or _core_aten_ops_exception_list, the error message will print two identical-looking summaries, leaving the user unable to see what actually conflicts. Consider including all differing fields, or at least _core_aten_ops_exception_list, in the summary. Fix this →

5. Re-enabling test_export_stages.py — removing the --ignore from both pytest.ini and pytest-windows.ini is straightforward. The explanation (testpaths pointed at a nonexistent dir since #12826, so nothing under export/tests was ever collected in OSS CI) is worth a maintainer confirming, since re-enabling a previously-uncollected file is the kind of change that can surface latent failures — CI on this PR is the real check here, and Dr. CI reports only an unrelated flaky QNN job.

Minor / maintenance

  • _UNORDERED_EDGE_CONFIG_FIELDS coupling (recipe.py:36)_edge_config_key treats every field not in this tuple as order-sensitive. If a future field on EdgeCompileConfig is another unordered op-list, it would silently be compared order-sensitively and could produce spurious "configs disagree" errors. Not actionable now, but a one-line comment noting "add new list-valued fields here" on the constant would help the next editor.

  • distinct: List[tuple[str, EdgeCompileConfig]] (recipe.py:350) — local-variable annotation, so it's never evaluated at runtime; the lowercase tuple[...] subscript is fine regardless. No issue, just confirming.

Summary

Solid, 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.
· export-recipe-bugfixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants