[Relax][ONNX] Support ConcatFromSequenc/SequenceInsert with new_axis=1#19361
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for the new_axis=1 attribute in the ONNX ConcatFromSequence operator by utilizing relax.op.stack. The test suite was updated to include cases for this new functionality. A review comment suggests adding a test case for concatenation along axis=1 to maintain the test coverage present in the previous version.
tlopex
left a comment
There was a problem hiding this comment.
Overall looks good to me. Please add a validation check for new_axis not in (0, 1), currently invalid values silently fall through to the concat branch. Also, the test coverage needs more cases: at minimum add (1, 0, [2, 32, 32]) and (1, -1, [32, 32, 2]) to cover basic stack-on-axis-0 and negative axis.
|
Last Commits summary:
thanks to @Dayuxiaoshui in #19360....i added one the tests he created and got inspired for other modifications as well :) |
apache#19361) ## Description This PR adds support for `new_axis=1` in the ONNX `ConcatFromSequence` operator, which was previously raising a `NotImplementedError`. *Note regarding the tracking issue:* The tracking issue listed this task as "SequenceInsert — Does not support inserting with new axis.", but i think it meant `ConcatFromSequence`. ONNX's `SequenceInsert` does not have a `new_axis` attribute, whereas `ConcatFromSequence` does and was throwing the "Insert new axis is not supported yet" error. This PR implements the missing feature. ## Changes: - Replaced the `NotImplementedError` in `ConcatFromSequence` with `relax.op.stack(inputs[0], axis=axis)` - Removed the `pytest.skip` from `test_concat_from_sequence`. - Parameterized the test to explicitly check both standard concatenation (`new_axis=0, axis=0` yielding `[64, 32]`) and stacking operations (`new_axis=1, axis=1` yielding `[32, 2, 32]`). ## Testing I tested the implementation via running: ``` pytest tests/python/relax/test_frontend_onnx.py::test_concat_from_sequence ``` and all tests passed: <img width="1044" height="89" alt="image" src="https://github.com/user-attachments/assets/25d6ad26-ad4b-4437-9fa5-e29efc9e0c9f" /> ## Reference https://onnx.ai/onnx/operators/onnx__ConcatFromSequence.html https://onnx.ai/onnx/operators/onnx__SequenceInsert.html partially addresses apache#18945
Description
This PR adds support for
new_axis=1in the ONNXConcatFromSequenceoperator, which was previously raising aNotImplementedError.Note regarding the tracking issue: The tracking issue listed this task as "SequenceInsert — Does not support inserting with new axis.", but i think it meant
ConcatFromSequence. ONNX'sSequenceInsertdoes not have anew_axisattribute, whereasConcatFromSequencedoes and was throwing the "Insert new axis is not supported yet" error. This PR implements the missing feature.Changes:
NotImplementedErrorinConcatFromSequencewithrelax.op.stack(inputs[0], axis=axis)pytest.skipfromtest_concat_from_sequence.new_axis=0, axis=0yielding[64, 32]) and stacking operations (new_axis=1, axis=1yielding[32, 2, 32]).Testing
I tested the implementation via running:
and all tests passed:

Reference
https://onnx.ai/onnx/operators/onnx__ConcatFromSequence.html
https://onnx.ai/onnx/operators/onnx__SequenceInsert.html
partially addresses #18945