Stop importing the SDK downloader to load the Qualcomm package - #22394
Stop importing the SDK downloader to load the Qualcomm package#22394shoumikhin wants to merge 3 commits into
Conversation
Deferring the SDK setup meant six modules now do this at import:
from executorch.backends.qualcomm import setup_qnn_sdk
which loads the package `__init__`, which imported the downloader from the sibling `scripts`
directory. That directory is not part of the installed package everywhere the backend is built, so
on those builds every one of those six modules fails to import:
ModuleNotFoundError: No module named 'executorch.backends.qualcomm.scripts'
Before this backend deferred its setup nothing imported the package `__init__` from inside the
package, so the missing directory never mattered. That is why it went unnoticed.
Nothing about loading the package needs the downloader. It is used in one branch of setup, the one
that fetches a prebuilt SDK, and that branch is unreachable when `QNN_SDK_ROOT` is already set or
the platform has no published SDK. So it is now imported inside that branch.
`is_linux_x86` moved here rather than being resolved lazily, because setup asks it before the
branch that needs a downloader, and the answer is a two line platform test. `install_qnn_sdk` and
`QNN_ZIP_URL` resolve through a module level `__getattr__`, so they are still ordinary module
attributes that callers and tests can replace.
Test plan:
package import, scripts present ok
package import, scripts absent ok, and setup_qnn_sdk() completes
QNN_SDK_ROOT set, scripts absent early return, downloader never imported
The new test covers the third case, which is the one that broke. It fails on the previous revision
with the ModuleNotFoundError above.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22394
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 1 Unrelated FailureAs of commit feb8b0b with merge base 90452c6 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
added
the
release notes: qualcomm
There was a problem hiding this comment.
Pull request overview
This PR removes an import-time dependency on the Qualcomm backend’s sibling scripts/ directory by deferring the SDK downloader import until it’s actually needed, preventing ModuleNotFoundError in builds that don’t package backends/qualcomm/scripts.
Changes:
- Replace module-level imports of the downloader with a lazy
__getattr__forinstall_qnn_sdk/QNN_ZIP_URL. - Define
is_linux_x86()inbackends/qualcomm/__init__.pyso platform gating can run without the downloader. - Add a regression test intended to ensure importing the package works even when the downloader module is unavailable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backends/qualcomm/init.py | Makes downloader symbols lazily resolved to avoid import-time failures when scripts/ isn’t packaged. |
| backends/qualcomm/tests/test_import_side_effects.py | Adds a test aimed at validating import behavior when the downloader modules are forced-missing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two review points, both right. The new test only checked that attributes existed after reloading with the downloader missing. It never called `setup_qnn_sdk()`, so it did not cover the case the change is about: a Linux x86 host with a preinstalled SDK, where setup takes its early return. It now forces that path and calls setup, which is what would raise if the downloader were imported again. And on a build with no downloader, where `QNN_SDK_ROOT` happens to be unset on Linux x86, the lazy attribute leaked a `ModuleNotFoundError` about a missing sibling directory. That tells the caller nothing they can act on. It is now a `RuntimeError` naming the variable to set, chained to the original so the cause is still visible.
|
Both of these were right, thanks. Fixed and pushed. The test did not exercise the case. You are correct that it only checked attributes existed after The leaked import error. Also right, and I reproduced it: on a build with no downloader where On the three red |
The catch was too wide. The downloader imports `requests`, so on a build where that is not
installed the missing dependency was reported as "This build cannot download a QNN SDK", pointing
the reader at packaging instead of at the real cause. Reproduced by hiding `requests`.
Narrowed to the downloader module itself, by name. A `ModuleNotFoundError` from anything else is
re-raised untouched:
requests missing ModuleNotFoundError naming requests
downloader missing RuntimeError naming QNN_SDK_ROOT
|
Right again, and I reproduced it before fixing. With the downloader present but The catch now only rewraps when the absent module is the downloader itself, checked by name. Anything Both verified by hiding each one in turn. |
| if not (error.name or "").startswith(f"{__name__}.scripts"): | ||
| raise |
|
Closing this in favour of #22392, which fixes the same problem and one more. This change keeps I checked both changes against an installed wheel in four packaging shapes, with Two ideas from here are worth keeping, and both are already in #22392: answering the Nothing outside the package imports |
|
Closing this as redundant. I branched it before #22392 merged and did not check, so it re-solves a #22392 removes the seven module level Everything worth keeping here is already on main:
I verified main handles the case this was opened for: with the One thing from #22392 is still open and is not covered by either change: |
Summary
Deferring the Qualcomm SDK setup left six modules importing the package itself:
That loads the package
__init__, which imported the SDK downloader from the siblingscriptsdirectory at module level. That directory is not part of the installed package in every build of this
backend, and where it is missing all six modules fail to import:
Anything importing one of those six then fails too, which surfaces as a test module that cannot be
collected rather than as a test failure.
Before the setup was deferred, nothing inside the package imported the package
__init__, so themissing directory never mattered. That is why this was not caught earlier: an installed source tree or
wheel has
scriptson disk, so the import resolves there.The change
Nothing about loading the package needs the downloader. It is used by one branch of setup, the one
that fetches a prebuilt SDK, and that branch cannot be reached when
QNN_SDK_ROOTis already set orwhen no SDK is published for the platform. So it is imported inside that branch instead.
Three details worth stating, because each one was a wrong turn first:
is_linux_x86is defined here now rather than resolved lazily. Setup asks it before the branchthat needs a downloader, so leaving it lazy pulled the downloader back in for every caller. The
answer is a short platform test.
install_qnn_sdkandQNN_ZIP_URLresolve through a module level__getattr__, so they stayordinary module attributes. A function local import is simpler but makes them unpatchable, which
silently broke four existing tests when I tried it.
RuntimeErrornamingQNN_SDK_ROOTratherthan letting a missing module escape, since a packaging detail is not something the caller can act
on. That rewrap is limited to the downloader module by name: the downloader imports
requests, anda missing dependency inside it has to keep reporting itself instead of being blamed on packaging.
Test plan
scriptspresentscriptsabsentQNN_SDK_ROOTset,scriptsabsentQNN_SDK_ROOT,scriptsabsent, Linux x86RuntimeErrornamingQNN_SDK_ROOTscriptspresent,requestsabsentModuleNotFoundErrornamingrequestsThe third row is the case that broke. The new test covers it by removing the module from
sys.modules, reloading, then forcing that path and callingsetup_qnn_sdk(). It fails on theprevious revision with the
ModuleNotFoundErrorabove.The last two rows were each verified by hiding the relevant piece and reading the error the caller
gets. The full test file passes with
scriptspresent and with it absent.Not covered
No Qualcomm device, so compiling a model for QNN end to end is not exercised here. What is tested is
which modules can be imported and what setup does in each of the states above.