Sparse segment sum sqrtn op #7149

codeislife99 · 2020-12-22T10:31:24Z

This PR is for adding support for sparse segment sum sqrtn OP (https://www.tensorflow.org/api_docs/python/tf/sparse/segment_sum?hl=bn) as a part of a larger effort to add sparse operator support. (#7125, #7126)
This operation is TF Specific and can't be implemented using existing ops.

codeislife99 · 2020-12-22T11:32:08Z

cc: @trevor-m @zhiics @comaniac @anijain2305 PTAL !

comaniac · 2020-12-22T19:13:07Z

The Relay part LGTM. However, since I'm not familiar with the implementation of those operators, I would ask @tkonolige and @mbrookhart to review this PR.

tkonolige

One thing I missed on previous PRs, is that this is a new op. That means we should follow https://tvm.apache.org/docs/contribute/code_review.html#deliberate-on-api-and-data-structures. @codeislife99 could you do the following:

write up the differences between tf, pytorch, and mxnets's versions of this operation.
If this operation is TF specific, can it be implemented just using existing relay operators?
Answer these same questions for all the other sparse PRs you currently have open.

Given we are adding so many sparse operations, I think there are a couple of questions to answer:

Should we create a new module for sparse operations in tvm.relay? i.e. tvm.relay.sparse
Instead of hard coding add these specific one-off sparse operations, should we take a high level approach like XLA?

@tqchen @jroesch @ANSHUMAN87 (please ping anyone else who has been working on sparse, not sure if I got everyone).

It may be worth discussing this on discuss.

src/relay/op/tensor/transform.cc

tkonolige · 2020-12-22T20:07:16Z

src/relay/op/tensor/transform.cc

+    .add_argument("indices", "Tensor", "The second tensor")
+    .add_argument("segment_ids", "Tensor", "The third tensor")
+    .add_type_rel("sparse_segment_sum", SparseSegmentSumRel)
+    .set_attr<TOpPattern>("TOpPattern", kInjective)


@mbrookhart Is kInjective the correct pattern here?

python/tvm/relay/op/transform.py

ANSHUMAN87 · 2020-12-23T16:06:06Z

One thing I missed on previous PRs, is that this is a new op. That means we should follow https://tvm.apache.org/docs/contribute/code_review.html#deliberate-on-api-and-data-structures. @codeislife99 could you do the following:
1. write up the differences between tf, pytorch, and mxnets's versions of this operation.

2. If this operation is TF specific, can it be implemented just using existing relay operators?

3. Answer these same questions for all the other sparse PRs you currently have open.
Given we are adding so many sparse operations, I think there are a couple of questions to answer:
1. Should we create a new module for sparse operations in `tvm.relay`? i.e. `tvm.relay.sparse`

2. Instead of hard coding add these specific one-off sparse operations, should we take a high level approach like XLA?
@tqchen @jroesch @ANSHUMAN87 (please ping anyone else who has been working on sparse, not sure if I got everyone).

It may be worth discussing this on discuss.

Thanks @tkonolige! I am in total agreement with the points you have shared. All these points need to be concluded.
I am already looking into these points as part of my plan for Sparse feature in TVM. Hopefully will be able to share a brief plan soon 🙂

antinucleon · 2020-12-23T19:22:03Z

One thing I missed on previous PRs, is that this is a new op. That means we should follow https://tvm.apache.org/docs/contribute/code_review.html#deliberate-on-api-and-data-structures. @codeislife99 could you do the following:

write up the differences between tf, pytorch, and mxnets's versions of this operation.

If this operation is TF specific, can it be implemented just using existing relay operators?

Answer these same questions for all the other sparse PRs you currently have open.

Given we are adding so many sparse operations, I think there are a couple of questions to answer:

Should we create a new module for sparse operations in tvm.relay? i.e. tvm.relay.sparse

Instead of hard coding add these specific one-off sparse operations, should we take a high level approach like XLA?

@tqchen @jroesch @ANSHUMAN87 (please ping anyone else who has been working on sparse, not sure if I got everyone).

It may be worth discussing this on discuss.

I am against to over document here. If the target is for TF frontend, why requires comparison to all framework? Link to the TF is fine.
What do you mean by high level approach like XLA?

tkonolige · 2020-12-23T20:08:01Z

I was just following guidelines that I saw here: [TOPI] Add embedding op and gradient #6794 (comment).
Sorry, not XLA, MLIR. They are taking a TACO approach to sparse matrices https://llvm.discourse.group/t/mlir-support-for-sparse-tensors/2020

tqchen · 2020-12-23T21:00:47Z

Thanks for the discussion so far. In this particular case I agree that having an API review is reasonable(check the possible references in different frameworks).

It would be great to keep the discussion to this PR and particular operator, so that we can move on constructively :)
The overall discussion of namespace and other approaches could use a separate thread

zhiics · 2020-12-30T03:03:57Z

@tkonolige Thanks for the suggestions. I agree that it might be worth to discuss if a sparse namespace is needed although there are already some sparse operators in the code base without using the new namespace.

However, I personally don't think we need to over complicate the process of implementing operators (e.g. comparing across all frameworks) as well, which is also not the process we have taken to implement other operators.

In addition, it may be impractical and/or inefficient to use existing operators to implement some TF/PT ops IMHO. Particularly, there are not that many sparse op in relay to compose a synthetic TF op.

@codeislife99 could you summarize the discussion so that we can have agreements on all points to move forward?

Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>

…r-tvm into SparseSegmentSumOp

Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>

codeislife99 · 2021-01-04T02:49:39Z

Context of these PRs: The goal of adding these sparse ops is to enable a customer to run their recommendation model which is currently getting split into multiple subgraphs because of our non-coverage of this op.

I had an offline discussion with the main reviewers but I will also try to summarize the conclusions from it and the comments here:

New namespace : Further discussion will be had in a separate thread, after the current (this one included) sparse ops PRs are merged, since there are a few already existing sparse ops without the namespace and if a new namespace is necessary all the current + previous sparse ops will be put in it.
Documentation : More documentation has been added
High level Approach like XLA: Discussion in a separate thread
This operation is TF specific.

codeislife99 · 2021-01-04T02:50:55Z

@tkonolige @mbrookhart Can I get a re-review on this PR ? I have added the TF Frontend code and some more documentation.

masahi · 2021-02-04T08:41:42Z

I'm interested in this discussion. I'm looking at how to support PyTorch EmbeddingBag op, which is used in Facebook DLRM model and which seems very similar to TF sparse segment sum op this PR is adding. The same ops exist in caffe2, too https://caffe2.ai/docs/sparse-operations.html So I'd say this op is not specific to TF and generally useful for recsys or possibly NLP use cases.

This op can be implemented by composing embedding op + reduce, and that is how ONNX exports PyTorch EmbeddingBag op. But that would involve materializing the embedding which is huge for DLRM. Both PyTorch and caffe2 use custom fused embedding lookup + on the fly reduce to efficiently implement these ops.

We cannot rely on automatic fusion of embedding + reduce, because our op fusion doesn't fuse any ops before a reduction op (if I remember correctly) cc @jwfromm @mbrookhart

codeislife99 · 2021-03-04T20:42:49Z

This functionality and the discussion has been addressed in #7562 . Closing this.

codeislife99 and others added 30 commits December 2, 2020 12:27

Fix trt Test

81e4c7f

Fixed stuff

8e2ce9a

Test TRT

d719346

Done

4e160e9

fix 0

3104113

Trigger Build

24116fd

Done

e8b223f

SparseReshapeOp

ff29e0c

Remove Build Module changes

fe3f7de

Merge

4fd2e57

Reset non-op changes

3f5de52

Remove stuff

a521c1b

More changes

04da7d4

Op Level 3

c32b2dd

Make Op changes only

fa5def3

Formatting Changes

dc8d1ce

Only Transform changes

2d48888

Correct Clang format version

2e017fd

Reset_to_clang-format-10

b7000ac

Merge Main

ea354d4

Remove SparseFill Changes

f14672a

PR stuff

0690155

Done

8c1f1f4

Done

2730eff

Add Brief;

58bae15

Black

b1cbce0

Address comments

bee77e0

Address PR COmments

5e08bcf

Change op Name to sparse_reshape

9954551

PR Comments

7508985

Ubuntu added 3 commits December 22, 2020 11:02

Black

3c73e40

Add docstring

526af81

Finish Test Cases

86558eb

tkonolige requested changes Dec 22, 2020

View reviewed changes

SparseSegmentSumSqrtN

43b5f6b

codeislife99 changed the title ~~Sparse segment sum op~~ Sparse segment sum sqrtn op Jan 2, 2021

Ubuntu and others added 12 commits January 2, 2021 02:05

Done

3ebdaa7

Add TF Frontend

31a1451

Op Level Testing

7edf7e9

CI

fb91d96

Update src/relay/op/tensor/transform.cc

e35122e

Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>

Merge branch 'SparseSegmentSumOp' of github.com:codeislife99/incubato…

3cd4e7c

…r-tvm into SparseSegmentSumOp

Update Description

2e4a1ba

Logging.h

166b61a

CI

126d2b0

Update src/relay/op/tensor/transform.cc

c3aea20

Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>

Address comments

3b64901

Comments

b7120f0

codeislife99 closed this Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sparse segment sum sqrtn op #7149

Sparse segment sum sqrtn op #7149

codeislife99 commented Dec 22, 2020 •

edited

Loading

codeislife99 commented Dec 22, 2020

comaniac commented Dec 22, 2020

tkonolige left a comment •

edited

Loading

tkonolige Dec 22, 2020

ANSHUMAN87 commented Dec 23, 2020

antinucleon commented Dec 23, 2020

tkonolige commented Dec 23, 2020

tqchen commented Dec 23, 2020 •

edited

Loading

zhiics commented Dec 30, 2020 •

edited

Loading

codeislife99 commented Jan 4, 2021

codeislife99 commented Jan 4, 2021

masahi commented Feb 4, 2021 •

edited

Loading

codeislife99 commented Mar 4, 2021

Sparse segment sum sqrtn op #7149

Sparse segment sum sqrtn op #7149

Conversation

codeislife99 commented Dec 22, 2020 • edited Loading

codeislife99 commented Dec 22, 2020

comaniac commented Dec 22, 2020

tkonolige left a comment • edited Loading

Choose a reason for hiding this comment

tkonolige Dec 22, 2020

Choose a reason for hiding this comment

ANSHUMAN87 commented Dec 23, 2020

antinucleon commented Dec 23, 2020

tkonolige commented Dec 23, 2020

tqchen commented Dec 23, 2020 • edited Loading

zhiics commented Dec 30, 2020 • edited Loading

codeislife99 commented Jan 4, 2021

codeislife99 commented Jan 4, 2021

masahi commented Feb 4, 2021 • edited Loading

codeislife99 commented Mar 4, 2021

codeislife99 commented Dec 22, 2020 •

edited

Loading

tkonolige left a comment •

edited

Loading

tqchen commented Dec 23, 2020 •

edited

Loading

zhiics commented Dec 30, 2020 •

edited

Loading

masahi commented Feb 4, 2021 •

edited

Loading