Skip to content

[Relax][ONNX] Support ConcatFromSequence new_axis=1 and refactor sequ…#19360

Closed
Dayuxiaoshui wants to merge 3 commits into
apache:mainfrom
Dayuxiaoshui:main
Closed

[Relax][ONNX] Support ConcatFromSequence new_axis=1 and refactor sequ…#19360
Dayuxiaoshui wants to merge 3 commits into
apache:mainfrom
Dayuxiaoshui:main

Conversation

@Dayuxiaoshui

Copy link
Copy Markdown
Contributor

Summary

  • Implement ONNX ConcatFromSequence with new_axis=1: expand_dims along axis, then concat on the same axis (matches ONNX Runtime).
  • Add _onnx_sequence_tuple_as_tensor_list for SequenceErase, SequenceInsert, and ConcatFromSequence.
  • SequenceInsert: coerce position to int and validate insert index.

Testing

@Dayuxiaoshui

Copy link
Copy Markdown
Contributor Author

cc @tlopex

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a helper function to standardize ONNX sequence handling and implements support for the new_axis attribute in ConcatFromSequence. It also adds bounds checking for SequenceInsert and expands test coverage. Review feedback suggests using more idiomatic Python for tuple conversion and improving efficiency in SequenceErase by using in-place deletion.

Comment thread python/tvm/relax/frontend/onnx/onnx_frontend.py Outdated
Comment thread python/tvm/relax/frontend/onnx/onnx_frontend.py Outdated

@tlopex tlopex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. A few comments before merge:

  1. _onnx_sequence_tuple_as_tensor_list feels unnecessary since it only does list(sequence). I’d just inline it.

  2. For the insert bounds check, please add a short comment that position == seq_len is valid since insert can append to the end.

  3. In SequenceErase, del items[position] is okay, but I’d prefer an assert position >= 0 for safety, or just keep the original list-comprehension version.

  4. Maybe also add a basic axis=0, new_axis=0 case.

@Dayuxiaoshui

Copy link
Copy Markdown
Contributor Author

cc @tlopex

Inline list(sequence) instead of _onnx_sequence_tuple_as_tensor_list.
Document that SequenceInsert allows position == seq_len for append.
Implement SequenceErase via list comprehension instead of del.

Add test_concat_from_sequence_axis0_new_axis0 (explicit axis for ORT)
and drop the redundant (0,0) parametrized case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants