Import profiling and threadpool test helpers from the extension module - #22032
Import profiling and threadpool test helpers from the extension module#22032shoumikhin wants to merge 1 commit into
Conversation
Three tests import a handful of private helpers, for profiling and for resetting the threadpool, from portable_lib. Those helpers are not part of what portable_lib is for. portable_lib exists to select the portable kernel build, as opposed to aten_lib, which selects the ATen kernel build. None of these three tests make that choice: they never reference aten_lib or a kernel mode, so importing portable_lib already fixed them to the portable build. Import the helpers from the extension module directly, which is the same build these tests were already getting, and says so plainly. Build dependencies are updated to match. No behaviour change. Every imported name is declared in the extension's type stub, and each of these files already imports torch before the extension, which the extension needs in order to load. Two other tests keep importing portable_lib, because they use it the way it is meant to be used: they try portable_lib and fall back to aten_lib to pick a kernel build at runtime. Test plan: - Confirmed all five imported names are declared in the extension type stub. - Confirmed torch is imported before the extension in all three files. - Confirmed none of the three reference aten_lib or a kernel mode, so no kernel selection behaviour is being changed. - black, flake8 and usort clean.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22032
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (7 Unrelated Failures)As of commit 194852a with merge base 8b93850 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was 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
This PR needs a
|
|
Closing this. Two independent reviews and my own follow-up checks agree it should not land, and the reasoning in the description above does not hold up. It contradicts a change I landed the day before. d64c302 (#21816) describes Three of the five names are documented as The "no behaviour change" claim was wrong. Importing And my justification for dismissing those was wrong. I checked the test sources for Windows references and found none, but One more correction: The underlying observation is still real: the profiling and threadpool controls have no public home, since |
Background: what portable_lib is actually for
ExecuTorch can build its Python extension against two different kernel sets, and this is a documented concept (see
ATen modeindocs/source/concepts.md):portable_libis the entry point for the first, andaten_libis the entry point for the second. Soportable_libis a kernel build selector, not a general utility module.The problem
Three tests import private helpers from
portable_libthat have nothing to do with choosing a kernel build:None of these three ever reference
aten_libor a kernel mode. They have no fallback and no choice, so importingportable_libalready pinned them to the portable build. The import suggests a kernel-build decision that the tests do not actually make.The change
Import those helpers from the extension module directly. That is the same build these tests were already getting, stated plainly instead of implied. Build dependencies are updated to match.
What is deliberately left alone
Two other tests keep importing
portable_lib, because they use it for exactly what it is for: they tryportable_lib, fall back toaten_lib, and record which kernel build they got.Those are correct as written and are not touched here.
Test plan
No behaviour change, verified rather than assumed:
pybindings.pyi).torchis imported before the extension in all three files, which the extension needs in order to load its dependencies.aten_libor a kernel mode, so no kernel selection behaviour changes.black,flake8andusortclean.Not verified locally: running these tests needs a compiled extension, which is not available on the machine this was prepared on. Left to CI.