Run the Qualcomm SDK setup on the paths that need it - #22395
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22395
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated Failures, 12 Unclassified FailuresAs of commit dc4d174 with merge base 2b3a32d ( NEW FAILURE - The following job has failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
7964207 to
598dff5
Compare
6dc4ce2 to
3d99666
Compare
598dff5 to
12e84c1
Compare
3d99666 to
48650b1
Compare
48650b1 to
ee4914a
Compare
ee4914a to
1545bf6
Compare
There was a problem hiding this comment.
Pull request overview
This PR moves Qualcomm QNN SDK setup (and the AMD MKLDNN guard) out of backends/qualcomm/__init__.py to eliminate import-time side effects, and reintroduces setup calls only on code paths that actually need a usable SDK (manager creation, context reading, SDK version queries, debugger tooling, and Android recipe selection).
Changes:
- Introduces
backends/qualcomm/utils/qnn_sdk_setup.pywith thread-safe, idempotent SDK setup anddisable_mkldnn_on_amd(). - Updates Qualcomm backend entry points to explicitly call SDK setup / AMD guard at runtime (instead of on import).
- Expands
test_import_side_effects.pyto enforce “no setup on import” and to validate setup behavior (idempotence, platform gating, empty SDK root handling, etc.).
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 rationale comments for Qualcomm SDK setup needs. |
| export/target_recipes.py | Triggers QNN SDK setup when constructing the Android recipe path. |
| backends/qualcomm/utils/utils.py | Removes import-time setup; adds setup/guard calls in runtime entry points and ensures AMD guard precedes tracing. |
| backends/qualcomm/utils/qnn_sdk_setup.py | New centralized, idempotent SDK setup + AMD MKLDNN guard implementation. |
| backends/qualcomm/utils/qnn_manager_lifecycle.py | Ensures SDK setup + AMD guard before creating/initializing a QnnManager. |
| backends/qualcomm/utils/check_qnn_version.py | Runs setup before reading QNN_SDK_ROOT to compute SDK build id. |
| backends/qualcomm/tests/test_import_side_effects.py | Adds stronger import-side-effect checks and detailed setup/guard behavioral tests. |
| backends/qualcomm/quantizer/validators.py | Removes import-time SDK setup side effects. |
| backends/qualcomm/quantizer/backend_opinfo_adapter.py | Retries loading SDK-backed opinfo after SDK setup to avoid “fallback forever”. |
| backends/qualcomm/debugger/utils.py | Ensures SDK setup before reading QNN_SDK_ROOT and running SDK tools. |
| backends/qualcomm/builders/node_visitor.py | Removes import-time SDK setup / AMD guard. |
| backends/qualcomm/init.py | Leaves package root deliberately empty to avoid import-time work and packaging-filelist fragility. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @lru_cache() | ||
| def get_backend_opinfo(backend: str, soc_model: QcomChipset): | ||
| # Retried here, rather than trusting the attempt made while this module was imported, because | ||
| # that ran before anything had made the SDK usable. Its result is cached, so deciding only | ||
| # there would drop every constraint check for the rest of the process. | ||
| if not _load_backend_opinfo(): | ||
| _warn_once_about_the_fallback() | ||
|
|
| if qnn_sdk_root: | ||
| print(f"[QNN] Using QNN SDK at {qnn_sdk_root} (from QNN_SDK_ROOT)", flush=True) | ||
| else: | ||
| print( | ||
| "[QNN] QNN_SDK_ROOT is set but empty, so the SDK is left to the caller", | ||
| flush=True, | ||
| ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
backends/qualcomm/utils/qnn_sdk_setup.py:64
setup_qnn_sdk()currently usesprint(...)for status output. This makes library calls write to stdout unconditionally and is inconsistent with the downloader’slogging-based messages (e.g.backends/qualcomm/scripts/download_qnn_sdk.py). Preferloggingso callers can control verbosity/handlers.
# Reported differently when empty, because printing it as a path would read as though a
# location had been found.
if qnn_sdk_root:
print(f"[QNN] Using QNN SDK at {qnn_sdk_root} (from QNN_SDK_ROOT)", flush=True)
else:
backends/qualcomm/debugger/utils.py:210
- Using
assertfor required environment variables is brittle (asserts can be disabled with-O) and produces less actionable failures. After callingsetup_qnn_sdk(), raise a real exception ifQNN_SDK_ROOT/ANDROID_NDK_ROOTare still missing so this fails reliably in optimized runs.
self.qnn_sdk = os.environ.get("QNN_SDK_ROOT", None)
self.ndk = os.environ.get("ANDROID_NDK_ROOT", None)
assert self.qnn_sdk, "QNN_SDK_ROOT was not found in environment variable"
assert self.ndk, "ANDROID_NDK_ROOT was not found in environment variable"
| try: | ||
| from executorch.backends.qualcomm.scripts.download_qnn_sdk import install_qnn_sdk | ||
| except ModuleNotFoundError as error: | ||
| # Only when the downloader itself is absent. A dependency missing from inside it is a | ||
| # different problem and is left to speak for itself. | ||
| if not (error.name or "").startswith("executorch.backends.qualcomm.scripts"): | ||
| raise | ||
| raise RuntimeError( | ||
| "This build cannot download a QNN SDK. Set QNN_SDK_ROOT to an existing " | ||
| "installation:\n" | ||
| " export QNN_SDK_ROOT=/path/to/qualcomm/sdk\n" | ||
| " export LD_LIBRARY_PATH=" | ||
| "$QNN_SDK_ROOT/lib/x86_64-linux-clang/:$LD_LIBRARY_PATH" | ||
| ) from error | ||
|
|
| try: | ||
| # Qualcomm QNN backend runs QNN sdk download on first use | ||
| # with a pip install, so wrap it in a try/except | ||
| # pyre-ignore | ||
| from executorch.backends.qualcomm.recipes import QNNRecipeType | ||
| from executorch.backends.qualcomm.utils.qnn_sdk_setup import setup_qnn_sdk | ||
|
|
||
| # The SDK is fetched here for a pip install that has not set one up by hand. It used to | ||
| # happen while the line above was imported, which meant every import of the backend | ||
| # downloaded an SDK whether or not one was wanted. | ||
| setup_qnn_sdk() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
backends/qualcomm/debugger/utils.py:210
- These environment checks use
assert, which is stripped underpython -Oand would allow execution to continue withNonepaths (leading to harder-to-debug failures later). Use explicit exceptions for required environment variables instead.
# Makes the SDK usable first, because this tool runs a binary from inside it. Setup used to
# happen while this module was imported, which set the variable read below as a side effect.
setup_qnn_sdk()
self.qnn_sdk = os.environ.get("QNN_SDK_ROOT", None)
self.ndk = os.environ.get("ANDROID_NDK_ROOT", None)
assert self.qnn_sdk, "QNN_SDK_ROOT was not found in environment variable"
assert self.ndk, "ANDROID_NDK_ROOT was not found in environment variable"
| try: | ||
| # Qualcomm QNN backend runs QNN sdk download on first use | ||
| # with a pip install, so wrap it in a try/except | ||
| # pyre-ignore | ||
| from executorch.backends.qualcomm.recipes import QNNRecipeType | ||
|
|
||
| # (1) if this is called from a pip install, the QNN SDK will be available | ||
| # (2) if this is called from a source build, check if qnn is available otherwise, had to run build.sh | ||
| if os.getenv("QNN_SDK_ROOT", None) is None: | ||
| raise ValueError( | ||
| "QNN SDK not found, cannot use QNN recipes. First run `./backends/qualcomm/scripts/build.sh`, if building from source" | ||
| ) | ||
| from executorch.backends.qualcomm.utils.qnn_sdk_setup import setup_qnn_sdk | ||
| except Exception as e: | ||
| raise ValueError( | ||
| "QNN backend is not available. Please ensure the Qualcomm backend " | ||
| "is properly installed and configured, " | ||
| "is properly installed and configured." | ||
| ) from e |
5cc9107 to
b43687c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backends/qualcomm/debugger/utils.py:210
- The QnnTool constructor uses
assertto validate required environment variables. Asserts are stripped underpython -O, which would skip these checks and likely lead to harder-to-diagnose failures later when paths are used. Prefer raising an explicit exception (matching the new pattern used in export_utils.py).
self.qnn_sdk = os.environ.get("QNN_SDK_ROOT", None)
self.ndk = os.environ.get("ANDROID_NDK_ROOT", None)
assert self.qnn_sdk, "QNN_SDK_ROOT was not found in environment variable"
assert self.ndk, "ANDROID_NDK_ROOT was not found in environment variable"
b43687c to
5e81122
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
export/target_recipes.py:159
- Catching ImportError here will also rewrite real backend failures (e.g. a missing third-party dependency inside the Qualcomm backend, or an internal ImportError) into a generic "backend not available" ValueError, hiding the actionable root cause. Consider only translating the specific case where the Qualcomm backend module itself is missing, and let other import errors propagate with their original traceback.
try:
# pyre-ignore
from executorch.backends.qualcomm.recipes import QNNRecipeType
from executorch.backends.qualcomm.utils.qnn_sdk_setup import setup_qnn_sdk
except ImportError as e:
# Only an import failure means the backend is not installed. Catching everything here
# would rewrite a real bug inside those modules as a missing backend and hide its
# traceback.
raise ValueError(
"QNN backend is not available. Please ensure the Qualcomm backend "
"is properly installed and configured."
) from e
backends/qualcomm/debugger/utils.py:210
- These environment-variable checks use assert, which is stripped under
python -Oand can lead to confusing downstream failures. Since this is validating user configuration, raise a real exception instead (similar to export_utils switching away from assert).
# Makes the SDK usable first, because this tool runs a binary from inside it. Setup used to
# happen while this module was imported, which set the variable read below as a side effect.
setup_qnn_sdk()
self.qnn_sdk = os.environ.get("QNN_SDK_ROOT", None)
self.ndk = os.environ.get("ANDROID_NDK_ROOT", None)
assert self.qnn_sdk, "QNN_SDK_ROOT was not found in environment variable"
assert self.ndk, "ANDROID_NDK_ROOT was not found in environment variable"
backends/qualcomm/quantizer/backend_opinfo_adapter.py:123
- _get_backend_opinfo_cached prints directly on exception, which can spam stdout in library usage and bypasses the module’s existing logging. Prefer logging (and capture the exception) so callers can control verbosity and still get diagnostics when needed.
@lru_cache()
def _get_backend_opinfo_cached(backend: str, soc_model: QcomChipset):
backend_type = getattr(backend_opinfo, backend.upper())
# For qnn 2.41, it only supports HTP backend
# It will support LPAI backend as soon as possible.
if backend == str(QnnExecuTorchBackendType.kLpaiBackend):
return _NoOpBackendOpInfo()
try:
return backend_opinfo.BackendOpInfo(backend_type, soc_model)
except Exception:
print(
f"The 'BackendOpInfo' APIs may not be available for this backend {backend}."
)
return _NoOpBackendOpInfo()
| if backend_type not in self._registry: | ||
| setup_qnn_sdk() | ||
| disable_mkldnn_on_amd() | ||
| qnn_manager = PyQnnManager.QnnManager(option) |
| setup_qnn_sdk() | ||
|
|
||
| self.qnn_sdk = os.environ.get("QNN_SDK_ROOT", None) | ||
| self.ndk = os.environ.get("ANDROID_NDK_ROOT", None) | ||
| assert self.qnn_sdk, "QNN_SDK_ROOT was not found in environment variable" |
| # Every example script builds this config before doing anything else, and the check | ||
| # below needs the SDK path. It used to be set as a side effect of importing the backend. | ||
| setup_qnn_sdk() | ||
| # A usable path, not merely the key being present, because everything below builds real | ||
| # paths from it. Raised rather than asserted: an assert is stripped under `python -O`, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
backends/qualcomm/export_utils.py:143
- os.environ.get("QNN_SDK_ROOT") is falsy for both “unset” and “set but empty”. Since other code paths now treat an empty QNN_SDK_ROOT as a deliberate caller choice, this message is clearer if it requires a non-empty path (e.g. “must be set to a non-empty SDK path”).
if not os.environ.get("QNN_SDK_ROOT"):
raise EnvironmentError("Environment variable QNN_SDK_ROOT must be set.")
| from executorch.backends.qualcomm.utils.qnn_sdk_setup import setup_qnn_sdk | ||
|
|
||
| setup_qnn_sdk() | ||
| try: |
| if not os.getenv("QNN_SDK_ROOT"): | ||
| raise ValueError( | ||
| "QNN SDK not found, cannot use QNN recipes. First run `./backends/qualcomm/scripts/build.sh`, if building from source" | ||
| ) |
|
@claude review this PR |
|
Claude finished @digantdesai's task in 2m 16s —— View job PR Review: Run the Qualcomm SDK setup on the paths that need it
This is a well-executed refactor. Moving SDK setup out of A few things worth a second look: 1.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
export/target_recipes.py:152
- The
except ImportErrorhere can still mask real bugs or missing dependencies inside the Qualcomm backend (e.g.ImportError: cannot import name ...orModuleNotFoundError: cpuinfo). That contradicts the comment about preserving real tracebacks. Narrow this toModuleNotFoundErrorand only rewrite the error when the missing module is actually underexecutorch.backends.qualcomm.
try:
# pyre-ignore
from executorch.backends.qualcomm.recipes import QNNRecipeType
from executorch.backends.qualcomm.utils import qnn_sdk_setup
except ImportError as e:
| assert not torch.backends.mkldnn.enabled | ||
| assert not recorded_install | ||
| # Nothing may reach the real installer from a test: it downloads about a gigabyte and | ||
| # rewrites the environment for the rest of the process. | ||
| assert not recorded_install |
### The problem The previous change stopped the Qualcomm SDK setup from running on import. Nothing calls it now, so this change calls it from the places that actually need an SDK. Setup used to live in `backends/qualcomm/__init__.py`, which Python runs on any import of the package, so importing it downloaded a toolchain, rewrote `QNN_SDK_ROOT` and `LD_LIBRARY_PATH`, and turned off a PyTorch CPU math library for the whole process. It is also not a safe home for code, because some builds leave `__init__.py` out and put an empty file there. ### The fix Both helpers move to `backends/qualcomm/utils/qnn_sdk_setup.py`, leaving `__init__.py` with only comments, so a real file and an empty stub behave the same. They are called from the paths that need a working SDK: the lowering entry points, the QNN manager, the context binary reader, the example config, the debugger tool, and the Android recipe helper. Both run before the model does: ```python setup_qnn_sdk() # can re-exec the interpreter, so never after a trace disable_mkldnn_on_amd() # calibration runs the model, so this comes first ep = torch.export.export(module, inputs, strict=True) ``` Order matters on both. On an old glibc the installer re-executes the interpreter, which would throw away an already-traced model. The AMD crash needs a real convolution, which calibration runs even though a trace does not. The CPU vendor behind that guard is read once per process, because `py-cpuinfo` caches nothing and spawns a subprocess per call. The setting itself is re-applied on every call, in case a caller turned it back on. And an empty `QNN_SDK_ROOT` now counts as set in setup. This one changes what users see, so it needs a deliberate yes: ``` export QNN_SDK_ROOT= # exported but empty, on Linux x86 before: falls through and downloads an SDK over the network after: says the SDK is left to the caller, and downloads nothing ``` A caller that exports the variable at all has taken charge of the SDK, often supplying it through `LD_LIBRARY_PATH`, so fetching a second copy over the top was wrong. Anywhere that builds a real path from the value still rejects an empty one. Happy to make it uniform instead if you would rather empty simply mean unset. ### Test plan `test_import_side_effects.py` passes, 53 tests, covering an empty and an unset SDK path, two threads at once, the call order above, the platform check across seven system and machine pairs, and that no module runs setup while being imported, including from a decorator. Each behaviour was checked by putting the bug back and confirming a test fails. `flake8` and `black` are clean. I have no Qualcomm device and no AMD host, so an end to end lowering and the crash itself are not covered.
The problem
The previous change stopped the Qualcomm SDK setup from running on import. Nothing calls it now,
so this change calls it from the places that actually need an SDK.
Setup used to live in
backends/qualcomm/__init__.py, which Python runs on any import of thepackage, so importing it downloaded a toolchain, rewrote
QNN_SDK_ROOTandLD_LIBRARY_PATH, andturned off a PyTorch CPU math library for the whole process. It is also not a safe home for code,
because some builds leave
__init__.pyout and put an empty file there.The fix
Both helpers move to
backends/qualcomm/utils/qnn_sdk_setup.py, leaving__init__.pywith onlycomments, so a real file and an empty stub behave the same. They are called from the paths that
need a working SDK: the lowering entry points, the QNN manager, the context binary reader, the
example config, the debugger tool, and the Android recipe helper. Both run before the model
does:
Order matters on both. On an old glibc the installer re-executes the interpreter, which would
throw away an already-traced model. The AMD crash needs a real convolution, which calibration
runs even though a trace does not.
The CPU vendor behind that guard is read once per process, because
py-cpuinfocaches nothingand spawns a subprocess per call. The setting itself is re-applied on every call, in case a
caller turned it back on.
And an empty
QNN_SDK_ROOTnow counts as set in setup. This one changes what users see, so itneeds a deliberate yes:
A caller that exports the variable at all has taken charge of the SDK, often supplying it through
LD_LIBRARY_PATH, so fetching a second copy over the top was wrong. Anywhere that builds a realpath from the value still rejects an empty one. Happy to make it uniform instead if you would
rather empty simply mean unset.
Test plan
test_import_side_effects.pypasses, 53 tests, covering an empty and an unset SDK path, twothreads at once, the call order above, the platform check across seven system and machine pairs,
and that no module runs setup while being imported, including from a decorator. Each behaviour was checked by putting the bug
back and confirming a test fails.
flake8andblackare clean.I have no Qualcomm device and no AMD host, so an end to end lowering and the crash itself are not
covered.