[JakeStevens]

Summary

exir/capture/__init__.py loads _capture.py, which imported executorch.exir.program and executorch.exir.verification.verifier at module scope. verifier.py, program/_program.py and _serialize/_serialize.py all import executorch.exir.capture._config, so each of them ran capture/__init__.py and looped back into itself while still half-initialised.

Normally exir/__init__.py loads capture first, so the cycle stays hidden. Under lazy imports (e.g. Cinder lazy imports on Python 3.14) the first import can enter through one of those modules instead, for example via backends/xnnpack/xnnpack_preprocess.py -> verifier, and fails:

ImportError: cannot import name 'EXIRATenDialectVerifierBase' from partially initialized module 'executorch.exir.verification.verifier' (most likely due to a circular import)

_capture.py uses ExirExportedProgram, _transform, HackedUpExportedProgramDONOTUSE and EXIRATenDialectVerifierBase only inside capture() and _capture_legacy_do_not_use(), so those imports move into the two functions, with a TYPE_CHECKING import for the return annotations. No public import paths change.

The separate exir/backend/backend_api.py <-> exir/program/_program.py cycle is not addressed here.

This PR was authored with AI assistance (Claude Code).

Test plan

New exir/tests/test_import_order.py (collected by OSS pytest, plus a Buck target) imports executorch.exir.verification.verifier, executorch.exir.program and executorch.exir._serialize._serialize in a fresh interpreter whose executorch.exir is a bare package, so exir/__init__.py doesn't run first. On main all three fail with a circular-import error; with this change all three pass. That stub imports eagerly, so on main it reports a different name than the lazy-import traceback above (e.g. EXIRATenDialectVerifier from _program.py); same cycle.

Smoke-tested capture() (with and without enable_aot). _capture_legacy_do_not_use() already fails on main with ExportedProgram.__init__() got an unexpected keyword argument 'verifier', and fails the same way with this change.

python -m unittest executorch.exir.verification.test.test_verifier executorch.exir.tests.test_passes executorch.backends.xnnpack.test.ops.test_add executorch.backends.xnnpack.test.ops.test_conv2d: same results as main (3 errors, test_to_out_variant_none_output and two test_qs8_conv*, which also error on main in my environment).

Not yet verified in a real Cinder lazy-imports Python 3.14 runtime.

cc @JacobSzwejbka @angelayi

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24799

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 5f26860 with merge base 3f08f3f ():

NEW FAILURE - The following job has failed:

• pull / android / run-emulator (gh)
  adb: device offline

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[JakeStevens]

@claude review effort=max

[claude]

Claude finished @JakeStevens's task in 22m 47s —— View job

---

Request Changes: the new local import still runs while verifier is loading, through the module-level EXIREdgeDialectVerifier() call at exir/verification/verifier.py:378, so this PR moves the import cycle to the last line of the module but does not break it (a two-line fix is below).

Detailed review

PR Review: #24799

Summary

This PR moves verifier.py's EdgeCompileConfig import under TYPE_CHECKING, plus a local import in _EXIREdgeDialectVerifier.__init__, so that importing verifier first no longer fails. The main problem: the module-level instantiation at line 378 still runs the cycle during verifier's own import. Also: a broader fix in _capture.py worth considering, no regression test, and a small BC change. Python couldn't be run in this job, so every finding comes from reading the import graph.

Code Quality

• The cycle still runs during verifier's own import (exir/verification/verifier.py:291, :378).

  • What happens: line 378 is a module-level EXIREdgeDialectVerifier() with the default class_only=False. So line 374 creates an instance, and __init__ runs the new import at line 291 while verifier is still loading.

  • The chain: when verifier is the first import (the case this PR targets), that import goes through capture/__init__.py:9 → _capture.py:17-18 → program/__init__.py:10 → _program.py. Those modules then import back from the half-initialised verifier at _program.py:89-93 and _capture.py:26.

  • Why it works today: every name they import happens to be defined above line 378.

  • How it breaks: the same ImportError comes back if someone does either of these:

    • adds a definition after line 378 that _program.py or _capture.py imports;
    • moves the call higher in the file.

    CI catches neither case, because a normal import runs exir/__init__.py:9 (and so capture) first.

  • Misleading wording: the comment at lines 40-41 and the PR title both say capture is no longer imported when the module loads, but it still is.

  • Fix: replace line 378 with:

    EXIRATenDialectVerifier(class_only=True)
    EXIREdgeDialectVerifier(class_only=True)

    • Line 378 presumably exists to register the EDGE dialect for load_verifier (exir/serde/serialize.py:690-697). Torch's _VerifierMeta registers a dialect when the class is created, so no instance is needed.
    • Today the instance path also registers OLD_EXIR_ATEN, through line 301. The first line above keeps that.
    • After this change, importing verifier first no longer touches capture.

  • If you keep the instantiation: add a comment at line 378 saying it triggers the capture import and has to stay the last statement.

  Fix this →

• The root cause is exir/capture/_capture.py:17-18, and other entry points still fail (a design point, not a regression).

  • Why other imports still loop: capture/__init__.py:9 loads _capture straight away, and _capture imports program at module scope. So any import of the small leaf module capture._config pulls in all of program. _program.py:31 and _serialize/_serialize.py:18 make that import as well.
  • Example: with the same stub, importing executorch.exir.program first still fails. The chain is _program.py:23 → _serialize.py:18 → capture/__init__.py:9 → _capture.py:17, which imports ExirExportedProgram from the half-initialised program package.
  • Fix:
    • _capture.py uses ExirExportedProgram, _transform, HackedUpExportedProgramDONOTUSE and EXIRATenDialectVerifierBase only inside function bodies (97/133/135 and 220/222/365/367) and in two return annotations (62, 170).
    • Move the imports at lines 17, 18 and 26 into those functions, with a TYPE_CHECKING import for the annotations. That fixes the verifier, program and _serialize entry points together.
    • verifier.py could then keep its module-level import, which also avoids the BC change below.
  • Not fixed either way: the backend_api.py:35 ↔ _program.py:26 cycle.
  • If you keep the narrow fix: reword "Breaking the cycle in _capture.py alone isn't enough". That statement only holds when line 26 is the only import moved.

  Fix this →

Testing

• No regression test.

  • CONTRIBUTING.md:345-346 asks for tests with bug fixes, and CI can't see this one (see above).

  • Suggested test, in exir/verification/test/test_verifier.py, running in a fresh subprocess:

    1. Put a bare executorch.exir module, with __path__ set to exir/, into sys.modules.
    2. Import executorch.exir.verification.verifier first.

    That checks in the stub from the test plan.

  Fix this →

• The test-plan stub can't produce the error quoted in the description. This is reasoned from the import graph, not run.

  • Why: only _capture.py:26 imports EXIRATenDialectVerifierBase at module scope, and it can run only after _capture.py:17-18 have imported a fully initialised _program. That in turn needs _program.py:89-93 to have already imported EXIRATenDialectVerifier, which is defined after EXIRATenDialectVerifierBase.
  • Consequence: under any eager import order (which the stub uses), the failure before this change is a different one, e.g. EXIRATenDialectVerifier at _program.py:89. The quoted error depends on real lazy-import behaviour.
  • Ask: please confirm the fix in the actual Cinder lazy-import runtime, or attach that traceback.

Backward Compatibility

• EdgeCompileConfig is no longer importable from executorch.exir.verification.verifier (verifier.py:42-43).
  • The module has no __all__, so from executorch.exir.verification.verifier import EdgeCompileConfig, or a mock.patch on that path, now fails.
  • Nothing in the repo does this, so the risk is only to out-of-tree code. The _capture.py approach above avoids the change.

Specific Comments

• exir/verification/verifier.py:40-41:
  • the cycle note is about 250 lines from the import it justifies (line 291), so that local import reads like a candidate to move back to the top. Put the note on the local import, as exir/lowered_backend_module.py:212-214 does.
  • Also spell out the full loop: verifier → capture/__init__ → _capture → program/_program → verifier.

Recommendation

Request Changes. The line 378 fix is what blocks approval: without it the PR moves the cycle instead of breaking it, and CI can't catch it coming back. The other items are not blocking.

Review steps

• Ran /pr-review: three independent reviewers, then consolidated and fact-checked their findings against the source.
• Traced the verifier → capture → program → verifier import chain by reading the imports of every module involved.
• Not done: running the stub reproduction or the tests. Python isn't allowed in this job; allowing python in --allowedTools would enable it.

[github-actions]

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.

[JakeStevens]

@claude review effort=max

[JakeStevens]

@claude review effort=max

[claude]

Claude finished @JakeStevens's task in 19m 10s —— View job

---

Request Changes: the _capture.py change breaks the cycle for all three tested modules, but OSS CI never runs the new regression test, because pytest.ini:52 skips exir/verification/test/test_verifier.py; moving the test into its own file under exir/tests/ fixes that.

Detailed review

PR Review: #24799

Summary

This PR moves _capture.py's imports from program, program._program and verifier into capture() and _capture_legacy_do_not_use(), and adds a subprocess test that imports verifier, program and _serialize._serialize without running exir/__init__.py. All three reviewers traced the module-scope imports and found no loop left for those modules. The one remaining problem is that OSS CI never runs the new test. Python couldn't be run in this job, so everything here comes from reading the code.

Testing

• OSS CI never runs the new test (exir/verification/test/test_verifier.py:38).

  • Where it runs today:
    • pytest.ini:52 ignores this file.
    • OSS CI runs plain pytest -n auto (.ci/scripts/unittest-linux-cmake.sh:35), and nothing in .ci/ or .github/ names the file.
    • Its only other runner is the Buck target at exir/verification/targets.bzl:62-71, which is defined only for Meta's internal build (:5-6).
  • Why it matters: no other test can see this cycle, because a normal import runs exir/__init__.py:9 first, and that loads capture. An OSS PR that moves a program or verifier import in _capture.py back to module scope would pass OSS CI.
  • Removing the ignore isn't enough: the relative import at line 25 (from ..verifier import ...) needs a parent package. exir/verification/test/ has no __init__.py, so pytest's default import mode loads the file as a top-level module, and that import fails.
  • Fix:
    • Move TestImportOrder and _IMPORT_WITHOUT_EXIR_INIT into their own file that OSS pytest collects, e.g. exir/tests/test_import_order.py (exir/ is in testpaths at pytest.ini:117).
    • Add a python_unittest for it in exir/tests/targets.bzl, like the error target at lines 107-115, with a dep on //executorch/exir:lib.
    • This also takes the test out of the verifier's test module, which fits better: two of the three modules it imports are program and _serialize._serialize.

  Fix this →

Specific Comments

• exir/verification/test/test_verifier.py:40: "Lazy imports can defer executorch/exir/init.py" isn't quite right. Under lazy imports exir/__init__.py still runs; what gets deferred is its imports, including capture at exir/__init__.py:9.
• exir/verification/test/test_verifier.py:31-32: exir = importlib.util.module_from_spec(spec) sets __path__, __spec__ and __file__ without running exir/__init__.py. That one line replaces both, and the types import is no longer needed.

Recommendation

Request Changes. Only the test location blocks approval. As written, the regression this PR guards against could come back without any OSS check noticing.

Separately, the PR says the fix hasn't been tried in a Cinder lazy-import runtime yet. That check is still worth doing before landing, because the new test only models an eager import order.

Review steps

• Ran /pr-review: three independent reviewers traced the import chains and reviewed both changed files. I merged their findings and checked each one against the source.
• Checked pytest.ini, .ci/, .github/ and the Buck targets to find where the new test runs.
• Not done: running the new test or the stub reproduction. This job needs approval for any python -c command; adding python to --allowedTools would allow it.

[JakeStevens]

@claude review effort:max

[claude]

Claude finished @JakeStevens's task in 3m 32s —— View job

---

Approve: the _capture.py change breaks the cycle for all three tested entry points and the new test now runs in OSS CI; what's left is two comment fixes in the test and one optional test case for the backend_api cycle that the PR leaves out of scope.

Detailed review

PR Review: #24799

Summary

This PR moves _capture.py's imports from program, program._program and verifier into capture() and _capture_legacy_do_not_use(), and adds exir/tests/test_import_order.py. The test imports verifier, program and _serialize._serialize without running exir/__init__.py first.

All three reviewers traced every module-scope import chain and found no loop left for those entry points. Every use of the moved names is inside a function that now imports it, including the early return ExirExportedProgram(ep, False). Nothing in the repo imports or patches those names on _capture. pytest.ini collects the new file. The findings below are all minor. Python couldn't be run in this job, so they come from reading the code.

Testing

• The test doesn't cover the cycle that's still open (exir/tests/test_import_order.py:27-31).

  • What happens: if executorch.exir.backend.backend_api is the first exir import, backend_api.py:35 → program/__init__.py:10 → _program.py:26 imports MethodProgramsPartitionerSpec and to_backend back from the half-loaded backend_api, and that fails.
  • Scope: the PR leaves this out on purpose. But the test comment says "each of these must import cleanly", which reads like full coverage, and under lazy imports this entry fails the same way as the bug being fixed.
  • Suggestion: add backend_api as an expectedFailure case or a TODO, so the gap shows up and the test flags it once fixed. Optional.

  Fix this →

Specific Comments

• exir/tests/test_import_order.py:25: "Lazy imports can defer executorch/exir/init.py" is inaccurate. exir/__init__.py still runs under lazy imports; what gets deferred is the imports inside it, including capture at exir/__init__.py:9. Two of three reviewers flagged this, and it's unchanged since the last review.
• exir/tests/test_import_order.py:16-17: exir = importlib.util.module_from_spec(spec) without exec_module sets __path__, __spec__ and __file__ the way a real package that hasn't run __init__ would. It replaces both lines and the types import.
• exir/capture/_capture.py:46-47: the cycle note sits on the TYPE_CHECKING block, about 25 and 130 lines away from the local imports it explains (:74, :181). Someone reading those imports may move them back to the top. A short note on the local imports would prevent that.

Recommendation

Approve. Nothing blocks. The PR notes the fix hasn't been tried in a real Cinder lazy-import runtime. That check is still worth doing before landing, because the test only models an eager import order where exir/__init__.py is skipped.

Review steps

• Ran /pr-review: three independent reviewers covered all three changed files. I merged their findings and checked each one against the source.
• Dropped the reviewers' concern that sys.executable -c can't find executorch under Buck. extension/pybindings/test/test_pybindings.py:385-386 already runs a subprocess that imports executorch.exir the same way.
• Not done: running the new test. This job needs approval for python; adding it to --allowedTools would allow that.