Fix non-inplace IfElse on numba backend#1765
Conversation
ricardoV94
left a comment
There was a problem hiding this comment.
Nice trick with the list, I was thinking we would need to use codegen due to numba limitations.
Left some comments.
We also need to work on the existing tests so they would have failed before the fix and pass now. We have to test both single and multi output and inplace or not
|
The list trick didn't work. I suspect that would happen. We need to use codegen for the non inplace version of ifelse with multiple outputs. Other cases will work fine. You can see some cases of codegen for the numba funcify |
Hey @ricardoV94, I have added codegen. A lot of tests are failing in |
Explanation for failing
|
|
Ah you need to set the borrow flags to allow pytensor to pass back inputs unalterated: import numpy as np
import pytensor
import pytensor.tensor as pt
x = pt.vector("x")
fn = pytensor.function([pytensor.In(x, borrow=True)], pytensor.Out(x, borrow=True), mode="NUMBA")
x_test = np.zeros(5)
fn(x_test) is x_testYou can check that without that the final function would have a deepcopy Op, using |
|
@ricardoV94, Firstly, I am sorry for the delay on this PR. I have been out sick I think the test is failing because Python object identity is lost at the graph level, before execution reaches Numba. Observed graph from the test is below From fn.dprint(): ViewOp [id A]
└─ Subtensor{i} [id B]
├─ if{inplace} [id C]
│ ├─ Gt
│ │ ├─ Sum{axes=None}
│ │ │ └─ x
│ │ └─ 0
│ ├─ ExpandDims{axis=0}
│ │ └─ x
│ └─ ExpandDims{axis=0}
│ └─ x
└─ 0
|
|
Subtensor should always return a view. You can add But why do you have a subtensor? Your |
06bf727 to
16e3a1c
Compare
|
I (force-)pushed some changes. This bug was causing pymc-devs/pymc#7993 to segfault. I also found that IfElse never views false branch variables, only true, due to a limitation in what PyTensor can handle for inplace |
| true_returns = ", ".join(true_names) | ||
| else: | ||
| true_returns = ", ".join(f"{name}.copy()" for name in true_names) | ||
| # Variables from the false branch are never aliased |
There was a problem hiding this comment.
This comment is too cryptic; can you put a small note why they aren't aliased? (And by "aliased" do you mean "views"?) You mentioned "a limitation in pytensor" in a comment on this PR but I really don't get it.
There was a problem hiding this comment.
View_map is always output: input_from_true_branch. More context in #1811
There was a problem hiding this comment.
Changed to # We only view true branch variables. False branch variables must always be copied.
# We only ever view (alias) variables from the true branch. False branch variables must always be copied.
More clear?
There was a problem hiding this comment.
It's more clear but I still don't understand why. It seems strange and arbitrary.
There was a problem hiding this comment.
It's how the Op is implemented. The limitation is part of the current inplace limitations that go beyond IfElse, as mentioned in #1811
cd9336c to
6c56fe2
Compare
Co-authored-by: Ricardo Vieira <28983449+ricardov94@users.noreply.github.com>
6c56fe2 to
4d98f00
Compare
Description
This PR fixes an issue in the
IfElsenumba, where outputs were returned as direct references to the input arrays instead of copies. This violated the semantics ofifelse, which guarantees that the returned value is a distinct object, even when both branches reference the same input.The fix ensures that each selected output is explicitly copied. This matches the behavior of the Python linker and prevents unexpected mutations when the NumPy arrays are assumed to be non-shared.
Related Issue
Checklist
Type of change