Skip to content

Sequence related ops#2249

Merged
wschin merged 6 commits intoonnx:masterfrom
BowenBao:bowbao/seq_related_ops
Sep 14, 2019
Merged

Sequence related ops#2249
wschin merged 6 commits intoonnx:masterfrom
BowenBao:bowbao/seq_related_ops

Conversation

@BowenBao
Copy link
Copy Markdown
Contributor

@BowenBao BowenBao commented Aug 22, 2019 •
edited
Loading

Following #2244. This PR adds the first few ops that supports basic sequence functionality in ONNX.

  • Complete sequence ops spec.
  • Update related ops with variadic inputs/outputs (concat, split, etc) spec.
  • Update shape inference for sequences.
  • Update onnx.helper for sequences.
  • Add test cases for new ops.

@BowenBao BowenBao requested review from a team as code owners August 22, 2019 00:24
gramalingam
gramalingam reviewed Aug 22, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
gramalingam
gramalingam reviewed Aug 22, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc
gramalingam
gramalingam reviewed Aug 22, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
@gramalingam
Copy link
Copy Markdown
Contributor

gramalingam commented Aug 22, 2019

Re. updating ops like concat/split: I feel it may be better to introduce new ops (e.g., ConcatSequence) allowing both the older form and newer form. Once implementations stabilize (e.g, if it becomes clear that there is no cost/overhead between the two versions), if necessary, we can deprecate the older form.

@ebarsoum ebarsoum added the topic: operator Issues related to ONNX operators label Aug 23, 2019
@spandantiwari
Copy link
Copy Markdown
Contributor

spandantiwari commented Aug 23, 2019

Re. updating ops like concat/split: I feel it may be better to introduce new ops (e.g., ConcatSequence) allowing both the older form and newer form. Once implementations stabilize (e.g, if it becomes clear that there is no cost/overhead between the two versions), if necessary, we can deprecate the older form.

I agree with this.

@spandantiwari
Copy link
Copy Markdown
Contributor

spandantiwari commented Aug 23, 2019

This is relevant to this PR and maybe even #2244 which brings in Sequence and Map types in ONNX. Do we want to rethink the name Sequence for the datatype and these new ops, given that it is also commonly used for data "sequences" for RNN models? This may cause confusion, if only initially, between the two because the Sequence type used here is not really what will be used for sequences for RNN models.

@BowenBao
Copy link
Copy Markdown
Contributor Author

BowenBao commented Aug 23, 2019

This is relevant to this PR and maybe even #2244 which brings in Sequence and Map types in ONNX. Do we want to rethink the name Sequence for the datatype and these new ops, given that it is also commonly used for data "sequences" for RNN models? This may cause confusion, if only initially, between the two because the Sequence type used here is not really what will be used for sequences for RNN models.

I agree. There is also an existing operator named ReverseSequence, which is unrelated to the Sequence type.

shinh
shinh reviewed Aug 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@shinh shinh left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work! I'm really happy to see this PR since our framework (chainer-compiler) already has similar ops for sequence. Let me ask two questions.

Comment thread docs/Operators.md
Comment thread docs/Operators.md
shinh
shinh reviewed Aug 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@shinh shinh left a comment

Choose a reason for hiding this comment

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

Let me leave one more comment

Comment thread docs/Changelog.md
@BowenBao
Copy link
Copy Markdown
Contributor Author

BowenBao commented Aug 24, 2019

Let me leave one more comment

Thanks for your comments! These are very helpful suggestions.

gramalingam
gramalingam reviewed Aug 27, 2019
View reviewed changes
Comment thread onnx/defs/shape_inference.h Outdated
gramalingam
gramalingam reviewed Aug 27, 2019
View reviewed changes
Comment thread onnx/defs/shape_inference.h Outdated
gramalingam
gramalingam reviewed Aug 27, 2019
View reviewed changes
Comment thread onnx/shape_inference/implementation.cc
gramalingam
gramalingam reviewed Aug 27, 2019
View reviewed changes
Comment thread onnx/shape_inference/implementation.cc Outdated
gramalingam
gramalingam reviewed Aug 27, 2019
View reviewed changes
Comment thread onnx/shape_inference/implementation.cc Outdated
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch from 82b81e9 to a980f1c Compare August 28, 2019 23:54
@BowenBao BowenBao changed the title [WIP] Sequence related ops Sequence related ops Aug 28, 2019
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch 3 times, most recently from 7ba3605 to cf32e9f Compare September 3, 2019 21:12
@BowenBao
Copy link
Copy Markdown
Contributor Author

BowenBao commented Sep 3, 2019

@prasanthpul - can we include this in 1.6?

@linkerzhang @houseroad @postrational - for review

gramalingam
gramalingam reviewed Sep 3, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
@prasanthpul prasanthpul added this to the 1.6 milestone Sep 4, 2019
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch from 1d8edf1 to dfa81c6 Compare September 6, 2019 21:58
@BowenBao
Copy link
Copy Markdown
Contributor Author

BowenBao commented Sep 7, 2019

cc @wschin for review as well.

spandantiwari
spandantiwari approved these changes Sep 9, 2019
View reviewed changes
Comment thread docs/Changelog.md
Comment thread docs/Changelog.md Outdated
Comment thread docs/Changelog.md Outdated
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch from dfa81c6 to 15d6506 Compare September 9, 2019 22:33
gramalingam
gramalingam reviewed Sep 12, 2019
View reviewed changes
Comment thread docs/Operators.md
@gramalingam
Copy link
Copy Markdown
Contributor

gramalingam commented Sep 12, 2019

Ideally, we should make all the sequence ops (except the Concat and Split versions) generic: that is, work for sequences of all kinds, not just sequences of tensors. Unfortunately, our type constraint specification language does not let us specify this currently. The type-constraint specification string allows us to say only things like "sequence(tensor(int))" and we need to generalize this to allow us to say "sequence(tensor(T))" where T denotes a type-variable (either constrained by other constraints or not). It would be great if we could do this, either as part of this PR or as a separate PR, depending on effort required.

gramalingam
gramalingam reviewed Sep 12, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch 2 times, most recently from a52e296 to 5c863d6 Compare September 12, 2019 23:52
gramalingam
gramalingam reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
gramalingam
gramalingam reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
@BowenBao BowenBao force-pushed the bowbao/seq_related_ops branch from 1094abd to 5cfd515 Compare September 13, 2019 18:13
@BowenBao
Copy link
Copy Markdown
Contributor Author

BowenBao commented Sep 13, 2019

Thanks for your review @gramalingam !

wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread docs/Changelog.md Outdated
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread docs/Changelog.md
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread docs/Changelog.md Outdated
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread docs/Changelog.md
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/backend/test/case/model/sequence.py Outdated
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/backend/test/case/model/sequence.py
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/helper.py Outdated
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
wschin
wschin reviewed Sep 13, 2019
View reviewed changes
Comment thread onnx/defs/sequence/defs.cc Outdated
@wschin wschin merged commit d7595f3 into onnx:master Sep 14, 2019
@vinitra-zz vinitra-zz mentioned this pull request Feb 1, 2020
Merged
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
@BowenBao @wschin
Sequence related ops (onnx#2249)
c989293 
* sequence related ops

* refine docs

* extend hasInputShape to Sequence

* refining naming and error checking

* refine descriptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gramalingam gramalingam gramalingam approved these changes

@wschin wschin wschin approved these changes

+2 more reviewers

@shinh shinh shinh left review comments

@spandantiwari spandantiwari spandantiwari approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic: operator Issues related to ONNX operators

Projects

None yet

Milestone

1.6

Development

Successfully merging this pull request may close these issues.

7 participants

@BowenBao @gramalingam @spandantiwari @shinh @wschin @ebarsoum @prasanthpul