Fix aten_roll for negative dims and shifts past the dimension length - #3024
Open
om singhal (Om-singhaI) wants to merge 1 commit into
Open
Fix aten_roll for negative dims and shifts past the dimension length#3024om singhal (Om-singhaI) wants to merge 1 commit into
om singhal (Om-singhaI) wants to merge 1 commit into
Conversation
`torch.roll` is circular. The shift is taken modulo the length of the dimension,
and a negative `dims` counts from the end. The lowering in
`onnxscript/function_libs/torch_lib/ops/core.py` does neither. It slices at
`dim_size - shift` for a positive shift and at `-shift` for a negative one, and
leans on `Slice` clamping to keep that in range, which only holds while the
shift stays inside a single wrap.
## A negative dim emits a model onnxruntime will not load
`_aten_roll_shift_and_dim_onnx` reads the dimension length with
`op.Shape(self, start=dim, end=dim + 1)`. For `dim = -1` that is
`start=-1, end=0`, and ONNX reads `end=0` as the absolute index 0, not as the
end of the shape. The range is empty, so `Shape` returns an empty tensor, and
that empty tensor is what reaches the `ends` input of `Slice`.
Unoptimized graph for `torch.roll(x, shifts=1, dims=-1)` on main, `x` of shape
`(2, 3)`:
```
Shape node_Shape_2 in=['x'] out=['val_2'] attrs={'end': 0, 'start': -1}
Sub node_Sub_4 in=['val_2', 'val_3'] out=['val_4']
Slice node_Slice_6 in=['x', 'val_5', 'val_4', ...] out=['val_6']
Concat node_roll in=['val_9', 'val_6'] out=['roll'] attrs={'axis': -1}
```
```
torch.roll(x, shifts=1, dims=-1)
torch [[2.0, 0.0, 1.0], [5.0, 3.0, 4.0]]
onnxruntime [ONNXRuntimeError] : 1 : FAIL : Node (node_Slice_6) Op (Slice)
[ShapeInferenceError] Incorrect or missing input value for starts and ends
```
Those nodes are this lowering rather than a torch decomposition. They are the
`Shape`, `Sub`, `Slice`, `Slice`, `Concat` chain from
`_aten_roll_shift_and_dim_onnx`, the output node keeps the `node_roll` name, and
editing `core.py` changes what onnxruntime returns, which is how every number
below was produced.
The export above uses `optimize=False` so the model gets as far as being saved.
With the default `optimize=True` it does not get that far: the empty tensor
trips the rewrite pass first and the export raises `PassError`. Same root cause.
Only `dim = -1` breaks this way. `dim = -2` on a rank 2 tensor gives
`start=-2, end=-1`, which is a nonempty range, and comes out correct.
## A shift past the dimension length is silently wrong
```
x = torch.arange(6, dtype=torch.float32).reshape(2, 3)
torch.roll(x, shifts=-4, dims=1)
torch [[1.0, 2.0, 0.0], [4.0, 5.0, 3.0]]
onnxruntime [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]]
torch.roll(x, shifts=7, dims=1)
torch [[2.0, 0.0, 1.0], [5.0, 3.0, 4.0]]
onnxruntime [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]]
torch.roll(x, shifts=14)
torch [[4.0, 5.0, 0.0], [1.0, 2.0, 3.0]]
onnxruntime [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]]
```
The boundary is narrow, which is why this went unnoticed. A shift of exactly one
dimension length is right because the rotation is the identity. A positive shift
between one and two lengths is also right, because `dim_size - shift` goes
negative and `Slice` reads a negative start as an index from the end, which
happens to be one wrap. It only goes wrong once a positive shift exceeds twice
the length, and for any negative shift whose magnitude is not a multiple of the
length. The last case above is the `dims=()` path, where the same thing happens
against the element count.
## The fix
`aten_roll` normalizes a negative dim against the rank, which is known at export
time, in the same shape `aten_unflatten` already uses:
```python
# PyTorch accepts negative dim as reversed counting
if dim < 0:
dim = self_rank + dim
```
`aten_roll_complex` needs its own version. The real representation carries a
trailing axis for the real and imaginary parts, so a negative dim resolves
against a rank one larger than the one torch sees. This matches
`aten_slice_complex` and `aten_squeeze_dim_complex`:
```python
if dim < 0:
# Account for the complex dimension in ONNX
dim = self_rank + dim - 1
```
Both helpers then take the shift modulo the length. ONNX `Mod` with `fmod=0`
gives the remainder the sign of the divisor, the same as Python, so
`(-shift) % length` is the split point for either sign of shift and any
magnitude:
```python
dim_length = op.Shape(self, start=dim, end=dim + 1)
slice_length = op.Mod(op.Constant(value_ints=[-shift]), dim_length)
```
The branch on the sign of the shift goes away in both helpers. The second
`Slice` was passing the total element count as its `ends` and relying on
clamping; it now takes the dimension length it already has. The graph for the
case above goes from 11 nodes to 8, and `Shape` now reads `start=1, end=2`.
## Testing
Added `test_roll_wraps_shifts_and_normalizes_negative_dims` and
`test_roll_complex_wraps_shifts_and_normalizes_negative_dims` to
`tests/function_libs/torch_lib/e2e_ops_tests.py`, following the `optimize=False`
pattern the tests around them use. Seven cases between them: negative dim,
positive and negative shifts past the dimension length, and the `dims=()` path
where the shift runs past the element count.
With only the `core.py` change reverted, all seven fail. Two fail at session
creation with the error quoted above, the other five with
`AssertionError: Tensor-likes are not close!`. With the fix all seven pass.
```
pytest tests/function_libs/torch_lib/e2e_ops_tests.py -k roll
7 passed, 100 deselected in 6.88s
pytest tests/function_libs/torch_lib/ops_test.py -k roll
6 passed, 2 skipped, 1844 deselected, 102 subtests passed in 4.87s
```
The existing OpInfo suite is unchanged, and it could not have caught this.
`sample_inputs_roll` uses only nonnegative dims, and its one large shift sample
rolls a `(5, 5, 5)` tensor by 10000, which is a multiple of 5 and therefore the
identity. Neither roll entry in `ops_test_data.py` carries a skip or an xfail,
so there was nothing to remove.
Also checked by hand and passing: several dims in one call, rank 3 with
`dims=-2`, a shift of zero, a shift that is an exact multiple of the length, a
dimension of length zero, and a dynamic dimension whose length is only known at
run time, which is the case where the `Mod` node has to survive into the graph
instead of folding away.
Environment: Python 3.10, torch 2.9.1, onnx 1.22.0, onnxruntime 1.23.2, macOS
arm64.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
torch.rollis circular. The shift is taken modulo the length of the dimension,and a negative
dimscounts from the end. The lowering inonnxscript/function_libs/torch_lib/ops/core.pydoes neither. It slices atdim_size - shiftfor a positive shift and at-shiftfor a negative one, andleans on
Sliceclamping to keep that in range, which only holds while theshift stays inside a single wrap.
A negative dim emits a model onnxruntime will not load
_aten_roll_shift_and_dim_onnxreads the dimension length withop.Shape(self, start=dim, end=dim + 1). Fordim = -1that isstart=-1, end=0, and ONNX readsend=0as the absolute index 0, not as theend of the shape. The range is empty, so
Shapereturns an empty tensor, andthat empty tensor is what reaches the
endsinput ofSlice.Unoptimized graph for
torch.roll(x, shifts=1, dims=-1)on main,xof shape(2, 3):Those nodes are this lowering rather than a torch decomposition. They are the
Shape,Sub,Slice,Slice,Concatchain from_aten_roll_shift_and_dim_onnx, the output node keeps thenode_rollname, andediting
core.pychanges what onnxruntime returns, which is how every numberbelow was produced.
The export above uses
optimize=Falseso the model gets as far as being saved.With the default
optimize=Trueit does not get that far: the empty tensortrips the rewrite pass first and the export raises
PassError. Same root cause.Only
dim = -1breaks this way.dim = -2on a rank 2 tensor givesstart=-2, end=-1, which is a nonempty range, and comes out correct.A shift past the dimension length is silently wrong
The boundary is narrow, which is why this went unnoticed. A shift of exactly one
dimension length is right because the rotation is the identity. A positive shift
between one and two lengths is also right, because
dim_size - shiftgoesnegative and
Slicereads a negative start as an index from the end, whichhappens to be one wrap. It only goes wrong once a positive shift exceeds twice
the length, and for any negative shift whose magnitude is not a multiple of the
length. The last case above is the
dims=()path, where the same thing happensagainst the element count.
The fix
aten_rollnormalizes a negative dim against the rank, which is known at exporttime, in the same shape
aten_unflattenalready uses:aten_roll_complexneeds its own version. The real representation carries atrailing axis for the real and imaginary parts, so a negative dim resolves
against a rank one larger than the one torch sees. This matches
aten_slice_complexandaten_squeeze_dim_complex:Both helpers then take the shift modulo the length. ONNX
Modwithfmod=0gives the remainder the sign of the divisor, the same as Python, so
(-shift) % lengthis the split point for either sign of shift and anymagnitude:
The branch on the sign of the shift goes away in both helpers. The second
Slicewas passing the total element count as itsendsand relying onclamping; it now takes the dimension length it already has. The graph for the
case above goes from 11 nodes to 8, and
Shapenow readsstart=1, end=2.Testing
Added
test_roll_wraps_shifts_and_normalizes_negative_dimsandtest_roll_complex_wraps_shifts_and_normalizes_negative_dimstotests/function_libs/torch_lib/e2e_ops_tests.py, following theoptimize=Falsepattern the tests around them use. Seven cases between them: negative dim,
positive and negative shifts past the dimension length, and the
dims=()pathwhere the shift runs past the element count.
With only the
core.pychange reverted, all seven fail. Two fail at sessioncreation with the error quoted above, the other five with
AssertionError: Tensor-likes are not close!. With the fix all seven pass.The existing OpInfo suite is unchanged, and it could not have caught this.
sample_inputs_rolluses only nonnegative dims, and its one large shift samplerolls a
(5, 5, 5)tensor by 10000, which is a multiple of 5 and therefore theidentity. Neither roll entry in
ops_test_data.pycarries a skip or an xfail,so there was nothing to remove.
Also checked by hand and passing: several dims in one call, rank 3 with
dims=-2, a shift of zero, a shift that is an exact multiple of the length, adimension of length zero, and a dynamic dimension whose length is only known at
run time, which is the case where the
Modnode has to survive into the graphinstead of folding away.
One case worth flagging for review. Rolling a dimension whose length is zero now
evaluates
Modwith a zero divisor, which the ONNX spec leaves undefined.onnxruntime returns the dividend, so both slices come back empty and
Concatreturns the input unchanged, which is what torch does. If you would rather not
depend on that, the alternative is to guard the modulo when the length is
statically zero. Separately,
torch.roll(torch.zeros(2, 0), shifts=3)with nodims still fails at
ReshapewithInvalid position of 0. That is unrelated tothis change and fails the same way before it.
Environment: Python 3.10, torch 2.9.1, onnx 1.22.0, onnxruntime 1.23.2, macOS
arm64.