Refactor: hoist conftest lazy imports that have no rationale#722
Merged
ChaoWao merged 1 commit intohw-native-sys:mainfrom May 8, 2026
Merged
Conversation
Seven `from simpler_setup.* import ...` / `import faulthandler` lines inside individual functions were carrying `# noqa: PLC0415` for no technical reason — the modules they imported are pure Python with no side effects, no circular import risk, and no significant import cost. Move them to the top of the file: - `simpler_setup.parallel_scheduler` (used in 4 spots) - `simpler_setup.pto_isa.ensure_pto_isa_root` - `simpler_setup.scene_test.clear_compile_cache` - stdlib `faulthandler` Lazy imports that *do* have a reason are kept: - `simpler.worker.Worker` (2 spots) — pulls in the nanobind C extension `_task_interface`; hoisting would slow every pytest startup including paths that never need a ChipWorker. - `xdist` — `try`/`except ImportError` probe for an optional dep; the whole point of importing inline is to handle the import failure. A side benefit: the SIGALRM handler no longer does a `from simpler_setup import parallel_scheduler` inside a signal context, which can deadlock on Python's import lock when the module has not been loaded yet. The handler now only references an already-loaded module attribute. Comment added at the handler explaining the constraint.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors conftest.py by hoisting several local imports, including faulthandler, parallel_scheduler, and clear_compile_cache, to the module level. This change is intended to prevent potential deadlocks that can occur when importing modules within signal handlers. I have no feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up cleanup to #721. Scanning the two files touched by that PR
turned up seven function-local imports each carrying
# noqa: PLC0415for no technical reason — the modules they import are pure Python, no
side effects, no circular risk, and no startup-cost impact. They were
just style debt.
Hoist these to the top of
conftest.py:simpler_setup.parallel_scheduler(used in 4 functions)simpler_setup.pto_isa.ensure_pto_isa_rootsimpler_setup.scene_test.clear_compile_cachefaulthandlerNet diff: -5 lines, all
# noqa: PLC0415of the removed imports gone.Lazy imports kept (intentional)
from simpler.worker import Worker(2 fixture sites):simpler.workerpulls the nanobind C extension
_task_interface. Hoisting would slowevery pytest startup, including
tests/utpaths that never constructa
ChipWorker.import xdist:try/except ImportErrorprobe for an optionaldep — the whole point of inline import is to handle the failure.
The full evaluation of every
# noqainconftest.pyandsimpler_setup/parallel_scheduler.py, with reasoning per line, is onthe PR conversation upstream.
Side benefit
The session-timeout SIGALRM handler installed in #721 used to run
from simpler_setup import parallel_schedulerinside the signalcontext. Doing imports inside a signal handler can deadlock on Python's
import lock if the module hasn't been loaded yet (rare, but possible
when the timeout fires before the dispatcher has reached
_dispatch_test_phases). The handler now only references analready-loaded module attribute. Comment added explaining the
constraint.
Testing
ruff checkclean (verified locally; pre-commit ran on commit)