Repository navigation
Import torch only when the Python runtime needs it, warn before outputs follow inputs, and upload bfloat16 without torch - #24321
shoumikhin wants to merge 9 commits into
Conversation
shoumikhin
added
the
release notes: api
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24321
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 1 Pending, 2 Unrelated FailuresAs of commit c3eb82b with merge base 75af78c ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job 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
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 20m 33s —— View job Needs Discussion: picking the output type on every call from Detailed reviewPR Review: #24321SummaryThis PR stops the portable Python bindings from importing PyTorch. Each call returns API Design
Backward Compatibility
Thread Safety
Testing
Code Quality
RecommendationNeeds Discussion This draft asks for discussion of the per-call detection and of changing the default without deprecation. That decision shapes the rest of the change. Whichever way it goes, the always-skipped aliasing test, the stale |
f914424 to
8903f4c
Compare
|
@claude review this PR |
|
Claude finished @shoumikhin's task in 5m 10s —— View job Needs Discussion: the output type still flips partway through a process depending on whether anything has imported torch yet, and a stable API's default changes with no deprecation step; the aliasing test is still always skipped and the Detailed reviewPR Review: #24321SummaryThis PR stops the portable Python bindings from importing PyTorch. Each call returns The three reviewers agreed on the main design problem: the output type depends on global import state that is read on every call. Some items from the earlier review are fixed: the policy test now checks All 8 files were reviewed. Nothing was built or run, so every finding comes from reading the code. The checkout is shallow, so the diff was taken against the PR's parent commit Thread Safety
Backward Compatibility
API Design
Testing
Code Quality
RecommendationNeeds Discussion The draft asks whether to change the default, and the reviewers agree that deciding the type on every call from |
|
@claude review this PR |
|
Claude finished @shoumikhin's task in 5m 17s —— View job Needs Discussion: the output type still changes partway through a process depending on whether anything has imported torch yet. The new LLM-runner setup (2b3c024) can be delayed indefinitely, and if it fails, its error is raised in unrelated main-thread code. Four items from earlier reviews are still open: the always-skipped aliasing test, the stale Detailed reviewPR Review: #24321SummaryThis PR stops the portable Python bindings from importing PyTorch. Each call returns Three independent reviewers covered all 8 files, and every finding below was re-checked against the code. Nothing was built or run. Since the last review, commit 2b3c024 is new, and the earlier open items are still open. Thread Safety
API Design
Testing
Code Quality
RecommendationNeeds Discussion The default still has to be decided: per-call detection from |
87e0d97 to
00ffa2e
Compare
shoumikhin
changed the title
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 32m 47s —— View job Request Changes: in bindings built without CUDA support, Detailed reviewPR Review: #24321SummaryThe PR makes the portable Python bindings import torch only when a call keeps returning Code Quality
API Design
Backward Compatibility
RecommendationRequest Changes The CUDA regression and the lost warning affect callers who change nothing, so fix them before merge. Decide the broken-torch behavior and the rule for calls without tensor inputs before the warning text ships, since it commits users to both. The |
00ffa2e to
315be40
Compare
shoumikhin
changed the title
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 49m 49s —— View job Approve: nothing blocks merge. Four minor items remain: Detailed reviewPR Review: #24321SummaryThis PR makes the portable Python bindings import torch only when a call returns Testing
API Design
Backward Compatibility
RecommendationApprove Nothing blocks merge. The |
The Python runtime checks each input against torch.Tensor through the torch it finds in sys.modules, and keeps the first one it finds. That can be a torch that another thread is still importing, which has no Tensor yet, or the None that blocks an import. If torch was not imported when the bindings loaded, for example with EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT set to executorch, calls then raise AttributeError, even ones with only NumPy inputs. Now the runtime uses torch only once it is fully imported, and keeps looking until then. Reading the import state of the module runs Python code, so the runtime holds a reference to the module before it reads it. Without that, code that removes the module from sys.modules during the read would leave the runtime with a freed object. Tested on Linux x86_64 with a source build: a new test makes the call while another thread is halfway through importing a torch package, and with sys.modules["torch"] set to None. Without this change both cases fail with AttributeError. Another case puts an object in sys.modules that removes itself when its import state is read; the call must not crash.
An ExecuTorchResult supports the buffer protocol but has no truth value or
number conversion of its own. So Python falls back to defaults that can give
wrong answers without an error: bool(result) is always True, and int() and
float() parse the raw bytes as text, so a uint8 result that holds 55 (the
character "7") gives 7.
Now each of them raises TypeError and says how to convert the result first:
bool(result)
# TypeError: ExecuTorchResult has no truth value. Convert it first, for
# example with numpy.asarray(result) or torch.from_dlpack(result).
Tested with the standalone no-ATen build: a new test checks that each of
them raises TypeError, and it fails without this change.
…warn before outputs follow inputs Importing executorch.runtime imports PyTorch whenever it is installed, even for a program that only passes NumPy arrays. Only setting EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT=executorch avoids that, and never loading PyTorch for NumPy callers would change what they get back. So that change has to be announced first. Without the setting, importing executorch.runtime no longer imports torch. The outputs stay as they are today: torch.Tensor whenever PyTorch is installed, and ExecuTorchResult without it. So a program with only NumPy inputs still loads torch, at its first call that returns a tensor. A future release will make tensor outputs follow each call's tensor inputs, including inputs kept from an earlier call: - any torch.Tensor input: torch.Tensor outputs; - only NumPy, other buffer, DLPack or ExecuTorchResult inputs: ExecuTorchResult outputs, even when PyTorch is installed; - no tensor inputs: torch.Tensor outputs only if torch is already imported. get_attribute() and bundled program verification count as calls without tensor inputs. The first call whose outputs will change shows one FutureWarning, which says how to keep torch.Tensor outputs or get ExecuTorchResult outputs now. A call that runs the method picks the type first, so with an error filter for this FutureWarning, such a call raises before it runs the method. A torch that is installed but fails to import now makes each call that needs it raise its own error, with a note on how to skip torch. Before, an ImportError meant ExecuTorchResult outputs, and any other error made the import of ExecuTorch fail. EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT still fixes the type: "executorch" never imports torch, and "torch" imports it when ExecuTorch is imported. The Buck target of the pybindings test now depends on executorch.runtime, which the new test imports. Tested on Linux x86_64 with Python 3.12 and a CUDA build of PyTorch: the pybindings, runtime, bundled program and CUDA DLPack suites have no failures that main does not have, and the standalone no-ATen job passes with and without PyTorch. Importing executorch.runtime takes 0.04 s and 18 MB of peak memory instead of 1.9 s and 506 MB. A new test runs each case of the rule in a fresh process. Its checks of the new behaviour fail on a build without this change; the other checks pin behaviour that must stay the same.
Without PyTorch, ExecuTorchResult.from_host() is how host data is copied
to a CUDA device for a method input. It reads the host data through the
Python buffer protocol, which has no format for bfloat16. So bfloat16 host
data could not be uploaded: an ml_dtypes bfloat16 array cannot export a
buffer, and its uint16 view uploads as uint16, which a bfloat16 input
refuses.
from_host() now takes a keyword-only dtype, such as "bfloat16", spelled the
way ExecuTorchResult.dtype spells it:
ExecuTorchResult.from_host(x.view(np.uint16), dtype="bfloat16")
This is the reverse of how a result of that dtype already comes out, so
dtype accepts data in the buffer format that such a result exports: the
format of that dtype, which int8 shares with qint8 and int32 with qint32,
or, for a dtype the buffer protocol cannot describe, unsigned integers of
the same width. Anything else raises ValueError, so data is never
reinterpreted by mistake. A CPU ExecuTorchResult source must match its own
dtype. Without dtype, nothing changes.
The lookup by name is shared with the one for torch dtypes, and stops at
UInt64, the last dtype that has a Python name. The lookup for torch dtypes
used to go on to the end of the dtype enum, which is past the end of the
table in every build, and much further in ATen builds. So an unknown torch
dtype raised a RuntimeError from the dtype table, instead of the ValueError
that the lookup was written to raise.
Tested on Linux x86_64 with a CUDA build and the standalone no-ATen build:
new checks of accepted data, refused data and an unknown dtype name fail
without this change, and bfloat16 data uploaded from 16-bit integers runs
through a CUDA method.
A call that imports torch to keep torch.Tensor outputs remembers when torch is missing, so later calls do not look for it again. That memory stuck when torch became available later: - It was checked before the check for an imported torch. So after a call found torch missing, a torch imported later was not used: calls with NumPy inputs still returned ExecuTorchResult outputs, while calls without tensor inputs returned torch.Tensor. - A None in sys.modules, which blocks an import, also set it. After the None was removed, no later call imported torch, so NumPy inputs gave ExecuTorchResult outputs, unlike in a fresh process. So a test suite that blocks torch in one test made later tests depend on their order. Now the memory is checked only while torch is not loaded, and a None in sys.modules makes only the calls that see it skip torch. The test that blocks torch now also runs without EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT, the only mode that reaches this code. It removes the None and expects the next call to import torch, and it imports torch after a call that found it missing.
The FutureWarning that announces the change of output types was shown
once per process. The flag that tracked it was set before the warning was
issued, so if the first such call ran under an ignore filter, for example
inside a library's warnings.catch_warnings() block, no later call warned,
even under simplefilter("always"). This warning is the only notice before
the default changes.
Issue it at every such call and let Python's warning filters decide what
shows. The default filter still shows it once per call site. The message
now says "returns" rather than "returned", since it is issued before the
method runs.
A test that captures stderr during a call now ignores this warning. It
passed before only because an earlier test in the same process had used
up the one-time warning.
When importing torch raised an ImportError, for example because one of its shared libraries is missing, the bindings used to return ExecuTorchResult outputs, as if torch were not installed. When the torch import moved to the first call that needs it, only a ModuleNotFoundError for torch itself counted as missing, so such a torch made every call raise. That was the only change in output type that had no warning period. Count every ImportError as missing again, so these setups keep working as they did. Any other error from importing torch is still raised, with a note on how to run without it. The import now goes through importlib. Before Python 3.13, a thread that waits for another thread's import of torch gets a KeyError from PyImport_ImportModule when that import fails, not the ImportError. So with PyImport_ImportModule, when 2 threads make their first calls at the same time, the waiting one would raise while the other returns ExecuTorchResult outputs. A new test makes the second call while the first one is importing a torch that then fails with an ImportError.
The announced rule said that a call without tensor inputs, such as get_attribute(), a bundled program check, or a method that takes no inputs, would later return torch.Tensor only if torch was already imported. So its type would depend on whether unrelated code imported torch first, and the warning told users to rely on that import order. Such calls keep returning torch.Tensor outputs whenever torch can be imported, as they do today, and the future release keeps that. They no longer warn, since their outputs will not change. Calls whose tensor inputs are not torch.Tensor still warn, as before. The warning, the README, the API reference and the Method.execute docstring describe the new rule. Tested on Linux x86_64: the output type test checks that calls without tensor inputs return torch.Tensor outputs without a warning, and it fails without this change.
…port Bindings built without CUDA support have no CUDA runtime of their own, so they wait for CUDA work through torch. Before torch was imported lazily, importing ExecuTorch imported it whenever it was installed. After the lazy import, a wait found torch missing unless something else had imported it, and failed with an error that blamed the build. That broke CUDA inputs from other libraries, such as CuPy, in set_inputs() followed by execute(), and in a call to a method that returns no tensor. A wait now imports torch, unless EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT is executorch, which keeps torch out. get_attribute() waits the same way. When torch is still not there, the error says that imported torch would also work, and the note on a torch that fails to import names the wait as a reason too. A test passes CUDA memory from the CUDA driver, so nothing else imports torch. In a fresh process each, set_inputs() with execute(), and a call to a method without outputs, must import torch. With the setting at executorch, the wait must fail with the new error and leave torch out. The test needs a GPU and bindings built without CUDA support, so CI skips it.
315be40 to
c3eb82b
Compare
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 57m 55s —— View job Approve, with two open points: the output type can only be chosen for the whole process, with no way to opt into the announced rule, and the new CUDA-wait test never runs in CI. Detailed reviewPR Review: #24321SummaryThe PR makes the portable bindings import torch only at the first call that needs it, warns at each call whose outputs will change from API Design
Infrastructure
Testing
Performance
Code Quality
RecommendationApprove Nothing blocks merge. Users will act on the FutureWarning text, so before it ships in a release, it is worth deciding on the per-object option, or at least on a setting value that applies the announced rule. The other items are local fixes. |
What is wrong today
Importing
executorch.runtimeimports PyTorch whenever it is installed, even when a program only passes NumPy arrays. That costs about 2 s and 500 MB of peak memory. Not loading it would change outputs: today NumPy inputs givetorch.Tensoroutputs whenever PyTorch is installed. So the switch is announced first, and some bugs are fixed:bool()of anExecuTorchResultis alwaysTrue, andint()andfloat()read its raw bytes as text.EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT=executorch, calls fail withAttributeErrorwhile another thread imports torch, or whilesys.modules["torch"]isNone. With a lazy import, every user would see this.What this change does
executorch.runtimeno longer imports PyTorch, unlessEXECUTORCH_PYBINDINGS_TENSOR_OUTPUT=torch. The first call that returns a tensor imports it, so outputs do not change yet.FutureWarningthat says how to choose. Calls without tensor inputs, such asget_attribute(), do not warn.bool(),int()andfloat()of anExecuTorchResultraiseTypeError.Nonefor it insys.modules, is ignored until torch can be imported.ImportErrorcounts as missing, as before. Other errors are raised by the calls that need torch, with a note on how to skip it, not by importing ExecuTorch.ExecuTorchResult.from_host()takes a keyword-onlydtype. For anml_dtypesbfloat16 arrayx:What changes later
torch.Tensortorch.Tensortorch.TensorExecuTorchResulttorch.Tensor, with the warningExecuTorchResulttorch.Tensortorch.TensorExecuTorchResultExecuTorchResultEXECUTORCH_PYBINDINGS_TENSOR_OUTPUT=executorchpicksExecuTorchResultfor every call and never loads PyTorch.Tested
On Linux x86_64, each commit was built with and without CUDA support. At each commit, the pybindings, runtime, no-ATen, bundled program and CUDA DLPack suites fail only where main fails too, and that commit's new tests fail without it. Tests that need a GPU and bindings without CUDA support, which CI skips, were run locally.