Stop the Qualcomm backend setup from running during an import - #22392
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22392
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 12e84c1 with merge base 90452c6 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Qualcomm backend Python package to eliminate import-time side effects (SDK download/env mutation/MKLDNN toggling) and to ensure the backend remains importable even when a packaging system synthesizes an empty __init__.py.
Changes:
- Move SDK/mkldnn setup helpers out of
backends/qualcomm/__init__.pyintobackends/qualcomm/utils/qnn_sdk_setup.py, and remove all module-scope setup calls from consumer modules. - Trigger SDK setup only at “backend actually starts / context binary is read / SDK build id queried” call sites.
- Rework
test_import_side_effects.pyto assert (via module source inspection) that importing key modules does not run setup, and to unit-test setup behavior explicitly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Updates dependency commentary to reflect setup now happening at backend-start rather than import. |
| backends/qualcomm/utils/utils.py | Removes module-scope setup and adds setup calls only in functions that actually touch QnnManager/context binaries. |
| backends/qualcomm/utils/qnn_sdk_setup.py | Introduces a dedicated, locked, idempotent SDK setup module plus AMD MKLDNN workaround. |
| backends/qualcomm/utils/qnn_manager_lifecycle.py | Moves setup to the manager creation point (backend start). |
| backends/qualcomm/utils/check_qnn_version.py | Ensures setup can run before querying SDK build id. |
| backends/qualcomm/tests/test_import_side_effects.py | Expands import-side-effect coverage and adds direct setup behavior tests. |
| backends/qualcomm/quantizer/validators.py | Removes import-time SDK setup. |
| backends/qualcomm/debugger/utils.py | Removes import-time SDK setup. |
| backends/qualcomm/builders/node_visitor.py | Removes import-time SDK setup. |
| backends/qualcomm/init.py | Makes package root deliberately empty to avoid packaging/import-path brittleness. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| loader = getattr(module, "__loader__", None) | ||
| get_source = getattr(loader, "get_source", None) | ||
| if get_source is None: | ||
| return "" | ||
| return get_source(module.__name__) or "" | ||
|
|
| try: | ||
| import cpuinfo | ||
| except ImportError: | ||
| raise ImportError("Please install the cpuinfo with pip install py-cpuinfo.") | ||
|
|
bd50d39 to
5c2ec82
Compare
|
Both review comments landed on an earlier version of this pull request. It has since been narrowed to only removing the six module level calls, so neither file it flagged is part of it any more. Both points were correct and are addressed in the follow-up, #22395, which is where that code now lives:
|
5c2ec82 to
7964207
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
backends/qualcomm/builders/node_visitor.py:11
- After removing the module-level calls in this PR, there are no remaining non-test call sites of setup_qnn_sdk() in the repo (a search only finds the definition and test usages). That means QNN compile flows will no longer automatically pick up QNN_SDK_ROOT / perform SDK setup, which contradicts setup_qnn_sdk()'s docstring (“Called by the code paths that need the SDK, so a caller does not have to”) and is likely a functional regression for existing users.
To keep imports side-effect free while preserving behavior, please add an explicit setup_qnn_sdk() call in the actual QNN compile entrypoint(s) (the code path that starts compilation / loads QNN libs), rather than at module import time.
from typing import Any, Dict, Optional, Tuple
import executorch.backends.qualcomm.python.PyQnnManagerAdaptor as PyQnnManager
import numpy as np
import torch
| if not _is_linux_x86(): | ||
| _sdk_ready = True | ||
| return | ||
|
|
||
| if not install_qnn_sdk(): | ||
| if not _install_qnn_sdk(): |
7964207 to
598dff5
Compare
### The short version
Importing the Qualcomm backend used to download a toolchain over the network and change
global settings. That made the package impossible to import in some builds. This moves the
setup off the import path.
### Why an import did real work
Every Python package has an `__init__.py`, and Python runs it the moment you import
anything from that package. In this backend it used to fetch the Qualcomm SDK (the vendor
toolchain), set `QNN_SDK_ROOT` and `LD_LIBRARY_PATH`, and switch off a PyTorch CPU math
library for the whole process. None of it is needed just to load the package.
### Two ways it broke
**1. The helpers disappear.** Six modules had this at the top:
```python
from executorch.backends.qualcomm import setup_qnn_sdk
setup_qnn_sdk()
```
Some build systems list a package's files one by one. They can leave `__init__.py` out and
put an empty file there instead, so everything defined in it is gone and all six fail:
```
ImportError: cannot import name 'setup_qnn_sdk' from 'executorch.backends.qualcomm'
```
**2. The whole backend disappears.** `__init__.py` also imported the downloader at the top:
```python
from .scripts.download_qnn_sdk import install_qnn_sdk # needs backends/qualcomm/scripts/
```
The same builds do not ship that `scripts` directory. Importing any submodule runs
`__init__.py` first, so nothing in the backend could be imported at all:
```
ModuleNotFoundError: No module named 'executorch.backends.qualcomm.scripts'
```
### The fix
Remove the seven module level calls, and import the downloader only inside the function
that installs an SDK. Whether a download is possible is now answered without it:
```python
def _is_linux_x86() -> bool:
return platform.system().lower() == "linux" and platform.machine().lower() in (...)
```
A Mac or an ARM machine returns straight away. If the downloader is missing on a machine
that would have downloaded, the error now says to set `QNN_SDK_ROOT` instead of naming a
missing module.
One of the seven calls is not SDK setup: `node_visitor.py` also called
`disable_mkldnn_on_amd()`, which avoids a crash on some AMD hosts. Nothing calls it after
this change, so this and the follow-up are best landed together.
### Test plan
`test_import_side_effects.py` passes, 12 tests. Three are new: the package imports with the
`scripts` directory absent, setup returns on a platform with no published SDK, and a
missing downloader names `QNN_SDK_ROOT`. All three fail if the top level import comes back.
Checked against an installed wheel with `__init__.py` emptied and `scripts` removed, in all
four combinations: 20 of 20 imports clean. Before this change the cases without `scripts`
fail all five. `flake8` and `black` are clean.
I do not have a Qualcomm device, so compiling a model end to end is not covered here.
598dff5 to
12e84c1
Compare
digantdesai
left a comment
There was a problem hiding this comment.
stamping to unblock the train
The short version
Importing the Qualcomm backend used to download a toolchain over the network and change
global settings. That made the package impossible to import in some builds. This moves the
setup off the import path.
Why an import did real work
Every Python package has an
__init__.py, and Python runs it the moment you importanything from that package. In this backend it used to fetch the Qualcomm SDK (the vendor
toolchain), set
QNN_SDK_ROOTandLD_LIBRARY_PATH, and switch off a PyTorch CPU mathlibrary for the whole process. None of it is needed just to load the package.
Two ways it broke
1. The helpers disappear. Six modules had this at the top:
Some build systems list a package's files one by one. They can leave
__init__.pyout andput an empty file there instead, so everything defined in it is gone and all six fail:
2. The whole backend disappears.
__init__.pyalso imported the downloader at the top:The same builds do not ship that
scriptsdirectory. Importing any submodule runs__init__.pyfirst, so nothing in the backend could be imported at all:The fix
Remove the seven module level calls, and import the downloader only inside the function
that installs an SDK. Whether a download is possible is now answered without it:
A Mac or an ARM machine returns straight away. If the downloader is missing on a machine
that would have downloaded, the error now says to set
QNN_SDK_ROOTinstead of naming amissing module.
One of the seven calls is not SDK setup:
node_visitor.pyalso calleddisable_mkldnn_on_amd(), which avoids a crash on some AMD hosts. Nothing calls it afterthis change, so this and the follow-up are best landed together.
Test plan
test_import_side_effects.pypasses, 12 tests. Three are new: the package imports with thescriptsdirectory absent, setup returns on a platform with no published SDK, and amissing downloader names
QNN_SDK_ROOT. All three fail if the top level import comes back.Checked against an installed wheel with
__init__.pyemptied andscriptsremoved, in allfour combinations: 20 of 20 imports clean. Before this change the cases without
scriptsfail all five.
flake8andblackare clean.I do not have a Qualcomm device, so compiling a model end to end is not covered here.