[shoumikhin]

1. Stop reusing overwritten inputs

Problem. Export lets later values reuse an input's planned memory, so a second run without new inputs gave wrong numbers silently:

method.set_inputs([x, y]); method.execute()   # right
method.execute()                              # wrong, silently

So did Module.plan_execute() and partial input lists.

Change. Method installs memory-planned inputs again from what the caller passed at every execute(); Module does the same for kept inputs a call omits. So inputs are read at each run: in-place edits are seen, a storage change raises "pass it again", and an input the method writes in place (state.add_(x)) no longer carries over, unlike repeated Method::execute() in C++. The README notes the limits for DLPack producers and clone_outputs=False results passed back in.

2. Say why a call was refused

The runtime logs why it refused a call but returns only a code, so exceptions said just error 0x10 or error: 0x22. Now they include the error lines the runtime logged during the call:

RuntimeError: Failed to load method forward, error: 0x22: Failed to load
/tmp/executorch_cuda_..._0.so with error: libgomp.so.1: cannot open shared object file ...

3. Keep the shape of bfloat16 results

A (2, 3) bfloat16 result showed up in the buffer protocol as 12 flat bytes. Now dtypes without a buffer format show as same-width integers in the real shape; DLPack still carries the real dtype. Pass a bfloat16 result back in as the result or its bytes, not np.asarray(result).

4. Report a dtype by its name

ExecuTorchResult.dtype now returns a string such as "bfloat16" instead of a NumPy dtype (which raised for bfloat16), and the new TensorInfo.dtype_name() returns the same name. The experimental dtype_name property is removed. TensorInfo.dtype() still returns the integer; its repr prints dtype=float32.

5. Docs

The API reference keeps torch tensors in and out as the main example. A short "Without PyTorch" section shows the fallback: NumPy arrays in, ExecuTorchResult values out, and DLPack (for example CuPy) for GPU data. The Method.execute docstring follows the same order.

Testing

On Linux x86_64 with an NVIDIA GPU:

• Two new tests in test_pybindings.py (inputs installed again at each run, and the reason in error messages), plus small edits to existing tests for the bfloat16 buffer view and dtype names. Both new tests, and most of the edited ones, fail on the base branch; all of them pass here.
• The existing pybindings tests, including the no-ATen file as CI runs it and the CUDA DLPack tests in both output modes: no new failures. A few tests fail here because a library is missing (quantized ops, pandas); this change does not touch them.
• Without PyTorch and libgomp, the CUDA load error now names the missing library.
• The basic torch example and the "Without PyTorch" example on the API reference page print the documented output. The second one, and its CuPy steps, also ran with PyTorch imports blocked.
• Not tested: the ATen build, macOS, and Windows.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24146

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 124aef2 with merge base 3a37515 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 5m 12s —— View job

---

Two behaviour changes need discussion before merge: Method now refuses a second execute() on nearly every exported model, and NumPy now gives bfloat16, float8 and quantized results as integers instead of raising. The other findings are minor.

Detailed review

PR Review: #24146

Summary

The PR stops reruns from reading overwritten planned inputs, adds the runtime's logged error lines to exceptions, keeps the shape of bfloat16 results in the buffer view, and reports dtypes by name. Two changes need discussion: the Method rerun refusal is much broader than the hazard it guards against, and NumPy reads of dtypes it lacks now quietly return integers. The rest are smaller points about the error-line capture and TensorInfo.__repr__.

Backward Compatibility

• pybindings.cpp:3360-3365, :3620-3626, :3660: Method refuses every second execute() once any input is memory planned, whether or not the plan reuses that memory.
  • Export plans inputs by default, so this hits nearly every model.
  • The usual benchmark or warm-up loop m.set_inputs(x); for _ in range(N): m.execute() used to work and now raises "already ran on its inputs" on the second pass.
  • PyModule solves the same problem by installing the kept inputs again (:2746-2761). PyMethod already keeps its input owners (borrowed_inputs_ and the rest), so it could do the same and stay compatible.
  • If refusing is the intended contract, call it out as a breaking API change in the release notes, not only as a fix. Otherwise, reuse the Module approach.
  • The concurrency test now accepts "already ran" as an expected error (test_pybindings.py), which hides the change rather than testing it.
  • Raised by all three reviewers.
• pybindings_executorch_result.h:236 (numpy_array) and :645-666 (buffer_format): np.asarray(out) / out.numpy() used to raise for bfloat16, float8 and complex32. Now they return uint16 / uint8 / uint32 arrays without any error.
  • np.asarray(out, dtype=np.float32) or .astype(np.float32) converts the bit patterns, not the values. Generic code such as np.testing.assert_allclose(np.asarray(out), ref) now compares garbage instead of failing.
  • The README documents this, but it trades a clear error for wrong numbers.
  • Keeping the shape needs only the buffer view to change. Consider keeping numpy_array / __array__ raising for dtypes whose buffer_format is nullptr.
  • The same change gives QInt8/QInt32 signed formats "b"/"i" with no scale or zero point. QUInt8 stays nullptr and falls back to "B", which is inconsistent and isn't in the PR description.
  • Raised by all three reviewers.
• pybindings.cpp:2129-2131, :2166: TensorInfo.__repr__ changes from dtype=Float to dtype=float32, and the PR description doesn't mention it.
  • It now goes through python_dtype(), which throws "Unsupported ExecuTorch dtype" for any enum value missing from its table (pybindings_executorch_result.h:71-75). The old toString never threw.
  • In the ATen build, c10::ScalarType has values after UInt64 (UInt1–UInt7, Int1–Int7, Float8_e8m0fnu, …). str(method_meta) on such a program would raise instead of printing.
  • I worked this out from the code; it was not run, and the ATen build was not tested.
• ExecuTorchResult.dtype changes from np.dtype to str, and dtype_name is removed with no alias. Code like out.dtype == np.float32, out.dtype.itemsize and out.dtype_name breaks. Grep found no callers in the repo, and the property is marked experimental. If ExecuTorchResult has shipped in a release, keep dtype_name as a deprecated alias for one release.

Code Quality

• pybindings.cpp:130-161, :4816-4829: the error-line capture is wider than the description implies.
  • Unrelated lines: every error-level line logged during the binding call is attached, with the first 1024 bytes kept. A recoverable error logged early in the call (for example by a backend probe or fallback) comes first and can push the real cause out of the message. THROW_INDEX_IF_ERROR also attaches these lines to plain index errors.
  • Calling thread only: lines logged on worker threads (threadpool, delegate threads) land in those threads' thread_local buffers and never reach the exception.
  • Unguarded bindings: the ExecuTorchResult bindings (:4425-4510) and _get_operator_names (:4616) have no call_guard. Lines they log stay in the top-level buffer. Each scoped call saves that leftover and puts it back afterwards, so it persists and would be attached to the next unscoped THROW_IF_ERROR. Low confidence: I found no unscoped throw site today.
  • Logging off: with logging compiled out (EXECUTORCH_ENABLE_LOGGING=OFF, or ET_MIN_LOG_LEVEL above Error), the feature silently does nothing.
• pybindings.cpp:2746-2761: Module now copies every kept planned input into planned memory before each call.
  • A loop that passes a large input once and then leaves it out now pays a full copy on every call.
  • There is also a third change in meaning beyond the two the description lists: a kept input now aliases the caller's live storage. An in-place edit between calls (y.mul_(2); m.forward([x])) is now picked up, where before the copy installed once was used. The "storage changed" check compares only pointer and size, so it doesn't catch this.

Testing

• test_refused_input_errors_say_why matches exact runtime log text ("Input 0 was expected to have type Tensor…"). It fails in any build without logging, and whenever the runtime rewords a message. Consider skipping it when logging is off, or matching less of the text.
• test_module_installs_kept_planned_inputs_again relies on the comment that default planning gives y's bytes to the output. If the planner stops doing that, the test still passes without testing anything. Assert that the overwrite actually happens, for example by running the inner method twice and checking that the result is wrong.
• test_method_refuses_to_rerun_on_consumed_inputs has no case showing that a method with no planned inputs (alloc_graph_input=False) can still run twice.
• There is no test for the 1024-byte cap, for an execute() failure that carries runtime lines, or for nested scoped calls.

Recommendation

Needs Discussion: decide whether Method should install its inputs again like Module instead of refusing, and whether numpy() / __array__ should keep raising for dtypes NumPy lacks. Both change what existing callers get without warning.

[shoumikhin]

Thanks for the review. Changes and answers:

1. Method now installs its memory-planned inputs again before a run that reuses them, the same way Module does, so set_inputs() once and execute() many times works and gives right numbers.

2. NumPy and dtypes it lacks: np.asarray() reads the buffer protocol before __array__, so on main it already returned integers for a bfloat16 result, as flat bytes with the wrong shape. Now the shape is right and __array__ matches it. The README says how to get real bfloat16 values (DLPack).

3. The rest, briefly: TensorInfo's repr cannot hit an unknown dtype, because the program format only carries the 30 dtypes in the table. dtype_name was experimental and never in a release. The error-line capture covers the calling thread only, as the commit message says. Installing kept inputs again costs what passing them again costs, and is needed for correct results.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 6m 7s —— View job

---

The fix now gives right numbers on reruns. But a rerun after set_inputs() reads the caller's tensors from the second execute() on and not on the first, and the bfloat16 change also affects float8, complex32 and qint results and a common way of passing results back in. Everything else is minor.

Detailed review

PR Review: #24146

Summary

The PR installs memory-planned inputs again before reruns, attaches the runtime's logged errors to exceptions, keeps the shape of bfloat16 results in the buffer view, and reports dtypes by name. The two items under Backward Compatibility change what existing callers get; the rest are smaller design and test gaps. Points already settled in the author's reply (TensorInfo repr, worker-thread logging, dtype_name never released) are left out.

Backward Compatibility

• When a rerun sees in-place edits is hard to predict (pybindings.cpp:3573, :3611-3622, :3652; README.md:84-88; raised by all three reviewers).
  • set_inputs() copies planned inputs once. execute() copies them again only when inputs_spent_ is set, and that flag is set after a run.
  • So m.set_inputs([x]); x += 1; m.execute(); m.execute() runs on the old x first and the new x second: two different results from one set_inputs. The README states this rule ("seen from the second execute on") but does not remove it.
  • Inputs that went through a conversion (a torch tensor dense in another dim order, :3446-3448) are installed again from the bindings' private copy (view->owner()). In-place edits to the caller's tensor are never seen for those, so the README's "from the objects the caller passed" is wrong for them.
  • A single rule would be easier to explain and test. Either install the inputs at every execute(), which costs the same as the copy already made from the second run on, or keep a snapshot taken at set_inputs().
  • The PR says on purpose that programs which write a planned input in place (state.add_(x)) now differ from a C++ runner calling Method::execute() repeatedly. Release notes should say so, because people use the bindings to check what a device runner will do.
  • Fix this →
• Dtypes NumPy lacks now give integers with no error, and the author's reply covers only bfloat16 (pybindings_executorch_result.h:52, :54, :236, :648-666; all three reviewers).
  • On main, only bfloat16 fell back to flat_buffer(). qint8, qint32, float8, complex32, bits* and quint* raised in both __array__ (through dtype()) and the buffer protocol (through require_buffer_format()).
  • Now all of them come out as same-width integers. np.asarray(out, dtype=np.float32) gives bit patterns with a plausible shape, and qint results lose their scale and zero point.
  • QInt8 and QInt32 get explicit signed formats ("b", "i"), but QUInt8 stays nullptr and falls back to "B". The PR description does not mention this.
  • Round-trip regression: method([np.asarray(bf16_out)]), or replaying a result saved with np.save, used to work, because the flat uint8 bytes are accepted as raw bytes for a bfloat16 input. Now the array is uint16, which a bfloat16 input refuses (README.md:167-169, :1615-1617).
  • Keeping the shaped view for bfloat16 only, and still raising for the others, would limit the change to what the PR describes. Either way, call out the round-trip change in the release notes.

API Design

• ExecuTorchResult.dtype changes from np.dtype to str (pybindings.cpp:4435-4438, pybindings_executorch_result.h:386). out.dtype == np.float32 now silently gives False, and np.empty(out.shape, out.dtype) raises for "bfloat16". Also, TensorInfo.dtype() returns an int while ExecuTorchResult.dtype returns a str. If ExecuTorchResult has never been released, this is fine; if it has, dtype needs the same deprecation path as dtype_name.

Code Quality

• Stale error lines stay in the buffer (pybindings.cpp:150-161). RuntimeErrorScope puts back whatever was in the thread's buffer before the call (outer_).
  • Error lines logged by bindings without call_guard stay in that top-level buffer, because nothing ever clears it. These bindings are the ExecuTorchResult methods (to_host, from_host, __dlpack__, around :4429-4503) and _get_operator_names.
  • Each guarded call then copies those lines into a heap std::string and puts them back, so the stale text lives for the life of the thread. Failures inside those unguarded methods also get no reasons attached.
  • Adding call_guard to those bindings, or clearing the top-level buffer once a scope ends at depth 0, fixes this.
• Lines are truncated at 1024 bytes with no marker. An error that was logged and recovered from earlier in the same call (for example input_tensor_meta on a non-tensor input) comes first and can push the real cause out of the message.

Testing

• No test exercises the new code paths together.
  • The leak test (test_pybindings_leak.py) always passes a full input list through __call__, so it never covers the state this PR keeps between calls: planned TensorPtrs, owners kept for omitted inputs, and inputs_ reused by Method.execute().
  • The fuzz test never calls execute() twice, and it skips partial Module calls (assume(not (bad == "too_few" and use_module))).
  • Nothing covers the failure branches at pybindings.cpp:3614-3621 or :2750-2760.
• The leak test can hide a leak. It measures growth in VmHWM, which only ever rises, and every case runs one after another in one process. A high transient peak in an earlier case hides up to that much growth in a later case. Measuring current RSS (VmRSS) after gc.collect(), or running each case in its own process, avoids this.
• The fuzz file fails at collection without Hypothesis. It imports hypothesis at module level (test_pybindings_fuzz.py:22-23). It is the only file in the repo that uses Hypothesis, which is pinned only in .ci/docker/requirements-ci.txt, and extension/ is in pytest's testpaths. So pytest extension/ fails at collection wherever Hypothesis is missing. pytest.importorskip("hypothesis") fixes this. Neither new file is in extension/pybindings/test/BUCK.
• Two earlier test concerns had no reply.
  • test_refused_input_errors_say_why matches runtime log text that exists only when ET_LOG_ENABLED is on.
  • test_module_installs_kept_planned_inputs_again / test_method_installs_its_inputs_again assume that default planning gives y's bytes to the output, and nothing asserts that the overwrite happens.

Recommendation

Needs Discussion: settle one rule for when a rerun reads the caller's inputs, and decide whether the integer buffer view should apply to dtypes other than bfloat16 (along with the round-trip change). Neither needs a large code change, but both change behaviour that users will notice.

Not reviewed in depth: docs/source/runtime-python-api-reference.rst (one-line change), and most of test_pybindings_no_aten.py and test_pybindings_cuda_dlpack.py outside their changed lines. Nothing was built or run.

[shoumikhin]

Thanks. Second round:

1. One rule now: Method.execute() installs memory-planned inputs again from what the caller passed at every run (only __call__, which sets and runs at once, skips the extra copy). An in-place edit made after set_inputs() shows on the very next run. A test covers it.

2. Error lines are now kept only while a binding call is running on that thread, so nothing logged outside a call can linger or reach a later exception.

3. Not changed, with reasons: ExecuTorchResult is not in any release yet, so its dtype type can still change. The integer view applies to every dtype without a buffer format, on purpose, so there is one rule. Passing np.asarray() of a bfloat16 result back in now raises a clear dtype error instead of reading bytes; pass the result itself or its bytes. The kept-input tests fail on the base branch with today's planner, and the pybind build always logs, so the log-text test is stable there.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 4m 59s —— View job

---

Nothing here blocks the merge. One new correctness gap remains, and only in the ATen build: an input whose shape is changed in place passes the storage check and is installed again with that new shape. Two test points from round 2 also still have no answer.

Detailed review

PR Review: #24146

Summary

The PR installs memory-planned inputs again at every execute(), attaches the runtime's logged error lines to exceptions, keeps the shape of results whose dtype has no buffer format, and reports dtypes by name. The single rule for reruns and the narrower error-line capture answer the earlier rounds. This review leaves out points the author settled: the dtype type, the integer view for every dtype without a format, the round trip through np.asarray, and logging always being on. What is left is one ATen-only gap, some small inconsistencies, and test gaps.

Code Quality

• ATen build: an in-place shape change gets past the storage check (pybindings.cpp:973-981, :3622-3624; raised by 2 of 3 reviewers; suspected, worked out from the code and not run).
  • BorrowedInput::validate compares only the data pointer and numel * element_size.
  • In the ATen build, inputs_ holds EValues that share the caller's at::Tensor. execute() now calls set_inputs(inputs_) again on every run, so it picks up whatever metadata the tensor has at that moment.
  • Example: m.set_inputs([x, y]); x.t_(); m.execute() on a square x. Storage and size don't change, so validation passes. copy_tensor_data (tensor_util_aten.cpp) is a plain memcpy that ignores strides, so the run gets untransposed values with no error.
  • x.unsqueeze_(0) on a static-shape input fails set_input instead, which leads to the next point.
  • Before this PR, planned inputs were copied once at set_inputs(), so neither case could happen. The portable build snapshots sizes and strides into TorchTensorView and is not affected.
  • Fix: have validate also compare sizes and strides, or have the ATen build keep its own alias of the tensor.
• A failed install inside execute() gives a misleading error (pybindings.cpp:3625-3627). It sets inputs_failed_ = true, so every later execute() says "The last set_inputs() … failed partway", although the user's set_inputs() succeeded. The message should cover the install that execute() does itself.
• Truncated error text can split a UTF-8 character (pybindings.cpp:4825-4828; raised by 2 reviewers; suspected).
  • copied = std::min(length, room - prefix) cuts at a byte count.
  • Log lines include file paths, such as the .so path in the PR's own CUDA example. A cut in the middle of a multi-byte character makes what() invalid UTF-8.
  • pybind then fails to decode it, so the user may see a UnicodeDecodeError or a garbled message instead of the intended RuntimeError.
  • Fix: back the cut up to the last character boundary.
• The quantized dtypes break the "one rule" (pybindings_executorch_result.h:52-54 against the comment at :645-647 and the README).
  • The stated rule is that dtypes without a buffer format show as same-width unsigned integers.
  • But QInt8 → "b" and QInt32 → "i" are signed formats, while QUInt8 stays nullptr and falls back to "B".
  • So np.asarray(qint8_result) returns an ordinary-looking int8 array with no scale or zero point.
  • Either set those two entries to nullptr or document them.
• Some ExecuTorchResult methods have no error scope (pybindings.cpp around :4452-4509). to_host, from_host and __dlpack__ are registered without call_guard. Since lines are now kept only inside a scope, runtime errors from CUDA copies in these methods never get their reasons attached.

Testing

• Two points from round 2 still have no reply (raised again by all 3 reviewers).

  • test_pybindings_fuzz.py:22-23 imports hypothesis at module level, and extension/ is in pytest.ini's testpaths. A plain pytest -n auto fails at collection wherever Hypothesis is missing; pytest.importorskip("hypothesis") fixes this. Neither new file is in extension/pybindings/test/BUCK.
  • The leak test only calls __call__ with a full input list. It never covers the state this PR keeps between calls: inputs_ installed again by execute(), and the planned TensorPtrs of kept Module inputs. It also measures VmHWM, which never goes down, so an earlier case's peak can hide a later case's growth.

• The new failure branches have no tests. Nothing covers:

  • an install failure inside execute() (:3624-3632)
  • an install failure for a kept Module input (:2751-2766)
  • the nested RuntimeErrorScope restore (:153-162)

  The fuzz test always passes full inputs through __call__, so it never reaches these either.

Performance

• set_inputs() followed by execute() now copies planned inputs twice: once in set_inputs (:3562) and again in the first execute(). Only __call__ skips the second copy. To avoid it, set_inputs() could leave the planned copy to the next execute(), or execute() could skip it on the first run after set_inputs().

Recommendation

Approve once the ATen shape check is fixed or explicitly scoped out, since the ATen build was not tested. The other points are minor.

Not reviewed in depth: the strategies in test_pybindings_fuzz.py past line ~110. Nothing was built or run.

[shoumikhin]

Thanks. On the confirmation round:

1. ATen build, in-place shape change: scoped out of this PR. The ATen bindings are not built or tested here, so I do not want to change ATen-only code blind. The fix is small (install from at_tensor.alias(), which keeps its own sizes and strides) and can follow with a build that covers ATen.

2. Small follow-ups, not blocking: the "failed partway" message after a failed install inside execute(), and cutting the 1024-byte error text on a UTF-8 character boundary.

3. Unchanged on purpose: qint8 and qint32 show their signed storage formats, as the commit message says. to_host, from_host and __dlpack__ raise their own messages and do not go through the macro that adds runtime lines. set_inputs() then execute() copies planned inputs twice so that every run reads the caller's inputs; __call__ copies once. The fuzz file imports Hypothesis directly because CI pins it and a skip would hide the tests.

[shoumikhin]

@claude review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[claude]

Claude finished @shoumikhin's task in 7m 3s —— View job

---

Nothing blocks the merge. One new case should be documented or handled first: a clone_outputs=False result passed back in as an input is now read again on every run, after the run itself has overwritten it. The rest are a small gap in the storage check, a docstring error, and test coverage.

Detailed review

PR Review: #24146

Summary

The PR installs memory-planned inputs again on every run, adds the runtime's logged error lines to exceptions, keeps the shape of results whose dtype has no buffer format, and reports dtypes by name. This round found one new behaviour change (borrowed results passed back in), a storage check that skips DLPack inputs, a docstring error, and test gaps that are still open. I left out points the author already settled or scoped out: the ATen shape check, the "failed partway" message, UTF-8 truncation, qint formats, the double copy after set_inputs(), the Hypothesis import, TensorInfo repr, and how far the error-line capture reaches.

The diff was taken against 4db778e, the parent of the PR's first commit, because the shallow checkout has no merge base with origin/main. Nothing was built or run.

Code Quality

• A borrowed result passed back in now feeds the run's own output into the next run, with no error (pybindings.cpp:3622-3633, :2751-2766; 2 of 3 reviewers; worked out from the code, not run).
  • A clone_outputs=False result points into the method's output memory. The comment at :4039-4042 says these results "always could see the next execution's values".
  • Example: r = method([x], clone_outputs=False)[0]; method.set_inputs([r]); method.execute(); method.execute().
    • The first execute() writes its output over r's bytes.
    • The second execute() installs r again, so it runs on that output instead of on the value that was passed.
    • For f(x) = x + 1, the second run gives x + 3. Before this PR it gave x + 2, because set_inputs() had copied r once.
  • Module does the same when such a result is kept and a later call leaves it out: forward([x, r]), then forward([x2]) reinstalls retained[1].planned, which points at r's memory.
  • No check catches this, because the address and size of r never change.
  • In effect, the run's own output becomes the next run's input. The PR says this kind of carry-over is gone ("no longer carries that write into the next run").
  • Fix: document it in the README next to the clone_outputs=False notes, or refuse/copy at set_inputs() any input whose data lies inside this method's output memory.
  • Fix this →
• DLPack inputs skip the storage check (pybindings.cpp:3486-3511, :2620-2638; 2 reviewers; confirmed by reading the code).
  • On the portable build, the DLPack branches of both PyMethod::set_inputs and PyModule::run_method never add a BorrowedInput or validation. The buffer and torch branches do.
  • This covers DLPack-only wrappers, CuPy arrays and every ExecuTorchResult, since all of them go through prefer_dlpack_input.
  • Example: a torch tensor behind a DLPack-only wrapper is reallocated with t.resize_(64, 64) between runs.
    • The reinstall then copies from the old storage, which the capsule keeps alive.
    • There is no crash, but the data is stale, and "a storage change raises 'pass it again'" does not hold.
  • Fix: watch the producer's data pointer and size for DLPack inputs too, or say in the README that the check covers only buffer and torch inputs.

API Design

• The Method.execute docstring misdescribes the return value (runtime/__init__.py:152-156; 1 reviewer; confirmed by reading the code).
  • It says the result is "a list of … torch.Tensor objects … otherwise ExecuTorchResult objects". Scalar and None outputs are returned as plain Python values (pybindings.cpp:4074-4083).
  • "Selects ExecuTorchResult objects either way" is not true for the ATen build. There, EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT is compiled out (#ifndef USE_ATEN_LIB at :168), so results are always torch tensors.
  • The same wording appears in docs/source/runtime-python-api-reference.rst:3.

Testing

• The leak test never reaches the state this PR keeps between runs (test_pybindings_leak.py:113-116; all 3 reviewers).

  • Every case calls executor(inputs). For a Method, that is call(), which passes inputs_just_installed=true (pybindings.cpp:3698), so the reinstall in execute() never runs.
  • No case omits a Module input, so kept planned TensorPtrs are not covered either.
  • It measures growth with VmHWM, which never goes down, so a high peak in an earlier case can hide growth in a later one. Measuring VmRSS after gc.collect() avoids this.

• The fuzz test never splits set_inputs() from execute() and never omits a Module input (test_pybindings_fuzz.py:316; 2 reviewers). So neither reinstall path is fuzzed across input kinds and layouts.

  • Separately, test_valid_inputs_match_numpy accepts any BufferError or ValueError that starts with "Input {i} " for a non-dense index (:233-235).
  • A bug that wrongly refuses a valid non-dense input with a dtype or shape error would still pass. Matching the specific refusal text would catch it.

• Neither new file is in extension/pybindings/test/BUCK, so internal CI will not run them. Open-source CI picks them up through pytest.ini testpaths.

• Untested paths:

  • a reinstall failure inside execute() (pybindings.cpp:3625-3632);
  • a reinstall failure for a kept Module input (:2757-2765);
  • a nested RuntimeErrorScope restoring the outer call's text;
  • the 1024-byte cap;
  • a method->execute() failure that carries runtime lines.

  test_refused_input_errors_say_why covers only one short reason from set_inputs.

Recommendation

Approve, once the borrowed-result case is documented or handled. It is the only point here that can give wrong numbers silently. The other points are minor and can follow.

[mergennachin]

@claude Is there a BC breaking API change and violating https://docs.pytorch.org/executorch/stable/api-life-cycle.html?

[claude]

Claude finished @mergennachin's task in 1m 27s —— View job

---

No, this PR doesn't break the API life cycle policy. Every API whose behaviour changes is in extension/pybindings and marked @experimental on main, and the policy lets those change without notice. The one stable entry point that reaches them, executorch.runtime.Method.execute, works the same for users who have PyTorch installed. One gap: I couldn't confirm that ExecuTorchResult is in no release, though that doesn't change the answer.

Details

What the policy says

From docs/source/api-life-cycle.md:

• An API marked Experimental "may be changed or removed without notice".
• An API with no marking counts as Stable. It "can be changed, but not in a breaking way", and has to be Deprecated for at least 2 minor releases before it is removed.

How the changed APIs are marked on main (cf6d4f2)

In extension/pybindings/pybindings.pyi, all of these carry @experimental("This API is experimental and subject to change without notice.") and a .. warning:: in the docstring:

• ExecuTorchResult
• ExecuTorchModule / ExecuTorchMethod
• TensorInfo / MethodMeta
• _load_for_executorch*

executorch.runtime (Runtime, Program, Method) has no marking, so it counts as Stable.

Each change against the policy

Change	API	State	Breaking?
ExecuTorchResult.dtype gives a str instead of np.dtype	ExecuTorchResult	Experimental	Yes, and allowed
ExecuTorchResult.dtype_name removed	ExecuTorchResult	Experimental	Yes, and allowed
Buffer/NumPy view of bfloat16, float8, qint and similar dtypes is now an integer array in the real shape, where before it was flat bytes or raised	ExecuTorchResult	Experimental	Yes, and allowed
ExecuTorchMethod.execute() installs planned inputs again on every run; a kept-input Module call does the same	ExecuTorchMethod / ExecuTorchModule	Experimental	Changes behaviour, and allowed
TensorInfo.dtype_name() added	TensorInfo	Experimental	No, it only adds
TensorInfo.__repr__ prints dtype=float32 instead of dtype=Float	TensorInfo	Experimental	A repr string isn't an API contract
Exceptions now include the error lines the runtime logged	all bindings	n/a	Same exception types; only the message text changes

The stable path: executorch.runtime.Method.execute

• It calls self._method(inputs, clone_outputs=...), so it always goes through __call__ with a full input list.
• __call__ installed every input on every call before this PR as well, and it skips the extra copy the PR adds (inputs_just_installed). So the rerun change can't reach this path, and the numbers it returns are the same.
• With PyTorch installed, the outputs are still torch.Tensor, so none of the ExecuTorchResult changes show up.
• Without PyTorch, the outputs are ExecuTorchResult, so a stable call can hand back a changed .dtype. The returned class is the experimental one, though, so the policy still allows this. The PR only rewrites the docstring of Method.execute; the signature stays the same.

Caveats

• Release status of ExecuTorchResult not confirmed. The author says neither ExecuTorchResult nor dtype_name has shipped in a release. version.txt on main says 1.6.0. I couldn't check the release/* branches: the checkout is shallow, and the GitHub API and web fetches weren't allowed in this job. Even if it did ship, it is marked experimental, so the policy is still met.
• Release notes. The PR has the release notes: api label. It would help users to list these there as experimental API changes (ExecuTorchResult.dtype → str, dtype_name removed, the integer buffer view, inputs installed again on execute()), even though the policy doesn't require a deprecation.
• Possible follow-up. If ExecuTorchResult is meant to become the stable torch-free output type, it may be worth settling its dtype type before the experimental marking comes off. Once the class is stable, changing it would need a deprecation.

I only read the code: docs/source/api-life-cycle.md, pybindings.pyi and runtime/__init__.py on main (cf6d4f2), and the PR diff. Nothing was built or run.

[mergennachin]

why are we supporting numpy now?

this looks like a new feature as opposed to fixing bugs.

also for normal torch, we do torch.from_numpy(np_array) and pass in torch.tensor type

[mergennachin]

okay claude answered it for me.

can you make the document read like numpy is a good fallback option if you don't have torch installed? having the numpy path as the default sample code is weird.

I would assume that most users will have torch installed and want a simple way, and we can have a separate fallback option for numpy.

[shoumikhin]

Done. The torch path is now the main example: torch tensors in, torch tensors out. NumPy moved to a short "Without PyTorch" section at the end. It is the fallback for users who do not have PyTorch, and it also shows DLPack (for example CuPy) for GPU data. The Method.execute docstring follows the same order.

[mergennachin]

@claude Did the pybinding API support numpy before and if not, why are we adding now? WHat's the behavior for normal torch library? Should we strive for consistency as opposed to adding new feature?

[claude]

Claude finished @mergennachin's task in 1m 2s —— View job

---

This PR doesn't add NumPy support: main (cf6d4f2) already takes NumPy arrays, buffers and DLPack objects as inputs. The doc change only describes what's already there.

Details

Did the pybindings accept NumPy before this PR?

Yes. On main (cf6d4f2), before this PR:

• README (extension/pybindings/README.md:13-19): "Portable bindings do not link against ATen or PyTorch. CPU inputs may be NumPy arrays or other dense objects implementing Python's buffer protocol … DLPack producers, including CuPy, can …"
• Code (pybindings.cpp): it has the buffer-protocol input path (py::buffer / py::buffer_info, around :297-615), NumPy scalar handling (:1525-1546) and DLPack import (:975).
• Tests (test_pybindings_no_aten.py:101-110): a test runs a program on np.array inputs and checks that torch was never imported.
• Stubs (pybindings.pyi:32): ExecuTorchResult is documented as read "through the buffer protocol, NumPy, or DLPack".

So the change you commented on (docs/source/runtime-python-api-reference.rst) describes behaviour that's already on main. Before, the page said only "accepts and returns torch.Tensor values", which was out of date. The PR adds no new input kinds. Its runtime changes are bug fixes to the existing paths: inputs installed again on reruns, error reasons, and the shape of bfloat16 results. The checkout is shallow, so I couldn't find the earlier PR that added torch-free input support.

Why the bindings take NumPy at all

The portable bindings can be built and run without PyTorch (EXECUTORCH_PYBINDINGS_TENSOR_OUTPUT=executorch, and test_import_does_not_load_torch). With no torch in the process, a torch.Tensor input can't exist. CPU data then has to come in through the buffer protocol (NumPy, memoryview, array.array) or DLPack. NumPy is the obvious choice there, not an extra feature.

How PyTorch behaves

• PyTorch ops don't accept an ndarray where a Tensor is expected. For example, torch.add(np.ones(2), 1) raises TypeError: add(): argument 'input' … must be Tensor, not numpy.ndarray. So an eager nn.Module or ExportedProgram.module() fed NumPy normally fails at its first op, and users write torch.from_numpy(x) first, as you say.
• PyTorch does interoperate, though: torch.from_numpy, torch.as_tensor, torch.from_dlpack, Tensor.numpy() and Tensor.__dlpack__.
• Inference runtimes, which are the closer comparison for the ExecuTorch runtime, mostly take NumPy: ONNX Runtime InferenceSession.run, the TFLite Interpreter, and Core ML predict.

Consistency or a new feature?

My recommendation:

• Keep torch.Tensor as the first-class, documented path when PyTorch is installed. It already is: with torch present, inputs may be torch tensors and outputs are torch.Tensor. The stable executorch.runtime.Method.execute behaves the same for those users (see my previous reply).
• Keep accepting NumPy and buffer inputs. Removing them would break the torch-free build, the only build where they're needed. They're also already on main, so removing them would be the real behaviour change.
• If you want the docs to stay torch-first, a small edit to this PR would do it. Make the example in runtime-python-api-reference.rst use torch.ones(...), and describe NumPy, buffer and DLPack inputs as "also accepted, and required when PyTorch is not installed" instead of leading with them. That matches the old page and still documents what the torch-free build needs.

I can push that doc edit if you and @shoumikhin want it.

I only read the code on main (cf6d4f2) and in the PR. Nothing was built or run. The PyTorch TypeError above is PyTorch's known behaviour; I didn't run it here.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 5m 7s —— View job

---

Nothing blocks the merge, and this round found nothing new that affects correctness. Since Method now reinstalls planned inputs, it keeps the caller's planned inputs alive until the next set_inputs(). The README should say so.

Detailed review

PR Review: #24146

Summary

The PR installs memory-planned inputs again at every run, attaches the runtime's logged error lines to exceptions, keeps the shape of results whose dtype has no buffer format, reports dtypes by name, and makes torch the main path in the docs with NumPy as the fallback.

Three reviewers went through 5fa57d1..HEAD, all 12 files. Most of what they raised had already been settled, scoped out or put off in earlier rounds, so I left it out:

• the bfloat16 integer view, including the dtype= casts and round trips
• in-place writes no longer carrying over to the next run
• call_guard on the ExecuTorchResult methods
• the planner assumption in the MulAdd reinstall tests
• the log-text test
• the leak test's VmHWM measurement
• the "failed partway" message

The torch-first docs that @mergennachin asked for read correctly against the code: the example APIs and the ExecuTorchResult.device member exist, and the docstring order matches.

Not covered in depth: the strategies in test_pybindings_fuzz.py. Nothing was built or run.

Performance

• Method now keeps planned inputs alive (pybindings.cpp, the removed is_memory_planned() gates in PyMethod::set_inputs, where the borrowed_inputs.emplace_back(...) calls are now unconditional; 1 reviewer; confirmed by reading the diff).
  • Before, a planned input was copied into planned memory and then released.
  • Now the bindings keep the caller's object, or their own converted copy, in borrowed_inputs_ / inputs_ until the next set_inputs(), so they can install it again at each execute().
  • Example: method.set_inputs([big]); del big no longer frees big's memory. The memory stays alive next to the planned copy.
  • The reinstall design needs this. Still, it is a new memory cost, and the README's rerun section should name it along with the per-run copy.

Recommendation

Approve. The one point above is a documentation follow-up, and the author has already listed the other follow-ups.

[JacobSzwejbka]

Did the pybinding API support numpy before and if not, why are we adding now? WHat's the behavior for normal torch library? Should we strive for consistency as opposed to adding new feature?

We are adding support for our pybindings without Torch for use in robotics applications where they can drop the Torch dependency. Existing users should see no change in behavior if you were using torch before everything will continue as normal, but for torchless users this lets them have io without bringing on a heavy aten dep if they dont need it.

[JacobSzwejbka]

@shoumikhin we have added too many test and test files here that are all very similar to one another please trim it down before landing. This is one of the areas we really need to push back against ai spam.

[shoumikhin]

Agreed on the memory note. Each execute() installs the planned inputs again, so the bindings have to keep them alive until the next set_inputs(). In the docs follow-up I will add a sentence to the README rerun section about this memory cost and the copy each run makes, together with the other follow-ups listed earlier.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

Thanks, trimmed: new test files 2 to 0, new tests 7 to 2, added test lines about 730 to 72. The leak and fuzz tests are gone. One test checks that inputs are read again on every run. One checks that errors say why a call was refused. The rest are small edits to existing bfloat16 and dtype tests.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 4m 44s —— View job

---

Nothing blocks the merge, and the trimmed diff adds no correctness problems. Two minor points are new: a comment that wrongly says planned and non-planned inputs now behave the same, and a Module error message that names an input the caller didn't pass in that call.

Detailed review

PR Review: #24146

Summary

The PR installs memory-planned inputs again at every run, adds the runtime's logged error lines to exceptions, keeps the shape of results whose dtype has no buffer format, reports dtypes by name, and makes torch the main path in the docs.

Three reviewers read all 9 changed files in origin/main...HEAD (merge base 3a37515), including the trimmed tests. I left out everything the author has already settled, scoped out or put off. Of what was new, two minor points held up when I checked them. Nothing was built or run.

Code Quality

• The reinstall comment is wrong for inputs the program writes in place (pybindings.cpp:3619-3621; 1 reviewer; confirmed by reading the code).
  • The comment says each run reads the caller's inputs "like inputs that are not memory planned". That holds only for inputs the program reads.
  • Take an input the program writes (state.add_(x), or a KV cache passed in as an input):
    • Not planned: the write-back lands in the caller's own storage, so the state carries over to the next run.
    • Planned: the write lands in the planned copy. The next execute() installs the caller's unchanged tensor again, so the state starts over.
  • So one model now behaves differently depending on whether alloc_graph_input planned its inputs. The PR description covers how this differs from the C++ Method::execute(), but not this split inside the bindings.
  • Fix: reword the comment, and add one line to the README rerun section saying this applies only to planned inputs.
• A failed reinstall of a kept Module input names an input the caller didn't pass (pybindings.cpp:2755-2765; 1 reviewer; confirmed by reading the code).
  • Take module.forward([x]) after an earlier forward([x, y]). If installing the kept input 1 again fails, the error is "Failed to set input 1 of method forward", the same text as for an input passed in this call.
  • Nothing in the message says the input was kept from an earlier call.
  • This is the Module counterpart of the "failed partway" message you already listed as a follow-up. It could go in the same change, for example as "Failed to install kept input %zu …".

Recommendation

Approve. Both points are minor and can go with the follow-ups you already listed.

[shoumikhin]

Thanks. Both points are fair, and both go into the follow-ups I listed before, not this PR. The reinstall comment gets reworded, and the README rerun section gets one line: a planned input that the program writes in place starts again from what the caller passed on every run, while an input that is not planned keeps the write. The Module error message will also say when the input was kept from an earlier call.

[shoumikhin]

@pytorchbot cherry-pick --onto release/1.6 -c fixnewfeature

[pytorchbot]

Cherry picking #24146

The cherry pick PR is at #24209 and it is recommended to link a fixnewfeature cherry pick PR with an issue.

Details for Dev Infra team

Raised by workflow job