Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement support for opset3 EmbeddingBag ops #546

Merged
merged 34 commits into from
Jun 8, 2020
Merged

Implement support for opset3 EmbeddingBag ops #546

merged 34 commits into from
Jun 8, 2020

Conversation

mvafin
Copy link
Contributor

@mvafin mvafin commented May 25, 2020 •
edited
Loading

Code:

  • Comments
  • Code style (PEP8)
  • Transformation generates reshape-able IR
  • Transformation preserves node names

Validation:

  • Unit tests
  • Transformation tests
  • MO IR Reader check

Documentation:

  • Supported frameworks operations list: N/A - since this is a PyTorch op, not specified in ONNX
  • Supported public models list
  • New operations specification
  • Guide on how to convert the public model: N/A
  • User guide update: N/A

Other:

  • Sample/Demo application to infer the model: N/A

@mvafin mvafin added the category: MO Model Optimizer label May 25, 2020
@mvafin mvafin requested a review from a team May 25, 2020 10:57
@mvafin mvafin requested a review from rkazants May 25, 2020 10:58
@mvafin mvafin force-pushed the feature/embedding-bag-opset-3 branch 2 times, most recently from c77070a to 7c4a242 Compare May 25, 2020 12:16
rkazants
rkazants previously requested changes May 26, 2020
View reviewed changes
Copy link
Contributor

@rkazants rkazants left a comment •
edited
Loading

Choose a reason for hiding this comment

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

why did you combine all changes into one commit? It is difficult to track what changes you moved and I have to check it manually. Please cherry-pick all commits as is.

@mvafin mvafin force-pushed the feature/embedding-bag-opset-3 branch from 7c4a242 to 2d5cb8f Compare May 26, 2020 08:32
@mvafin mvafin dismissed rkazants’s stale review May 26, 2020 08:41

Pushed initial commits

@rkazants
Copy link
Contributor

why did you combine all changes into one commit? It is difficult to track what changes you moved and I have to check it manually. Please cherry-pick all commits as is.

@rkazants rkazants closed this May 26, 2020
@mvafin mvafin reopened this May 26, 2020
@mvafin mvafin force-pushed the feature/embedding-bag-opset-3 branch from 907cf03 to 2938f6f Compare May 26, 2020 10:52
@jane-intel
Copy link
Contributor

Fill in the description, please. If PR is not ready -- please convert it to draft.

@mvafin mvafin requested a review from a team June 1, 2020 11:54
mvafin and others added 14 commits June 1, 2020 16:15
@mvafin
[MO] Implement EmbeddingBag_3
90bf6c6
@rkazants @mvafin
Transform dynamic sub-graph of Wide and Deep into EmbeddingSegmentsSum
bb897c4 
- Expressed SparseWeightedSum sub-graph through EmbeddingSegmentsSum
- Removed experimental SparseWeightedSum layer
- Implemented tests for the transformation

Signed-off-by: Roman Kazantsev <[email protected]>
@mvafin
Fix EmbeddingBag shape infer
3a86aff
@rkazants @mvafin
Fix EmbeddingSegmentsSum transformation for Wide and Deep
40295be 
Signed-off-by: Roman Kazantsev <[email protected]>
@rkazants @mvafin
Fix EmbeddingSegmentSum replacer after ports swap
725a1bb 
Signed-off-by: Roman Kazantsev <[email protected]>
@rkazants @mvafin
Update package_BOM.txt
f4bcd88 
Signed-off-by: Roman Kazantsev <[email protected]>
@mvafin
Add unit tests for EmbeddingXXX shape infer
05b1faf
@mvafin
Fix ATen resolver
973e9b5
@mvafin
Remove deleted files from BOM
4bf92b2
@mvafin
Add opset version to embedding_bag
75b5ec4
@mvafin
Use base class for EmbeddingBag
e9cf0fa
@mvafin
Fix per_sample_weights case
adab635
@rkazants @mvafin
Fix EmbeddingSegmentsSum transformation
af14345 
Signed-off-by: Roman Kazantsev <[email protected]>
@mvafin
Fix EmbeddingBag checks
0f202f9
lazarevevgeny
lazarevevgeny suggested changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@lazarevevgeny lazarevevgeny left a comment

Choose a reason for hiding this comment

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

Found an issue which breaks reshape-ability of the model.

jane-intel
jane-intel requested changes Jun 5, 2020
View reviewed changes
model-optimizer/extensions/back/RemoveConstToResult.py Outdated Show resolved Hide resolved
model-optimizer/extensions/front/onnx/aten_ext.py Show resolved Hide resolved
model-optimizer/extensions/ops/embedding_bag.py Outdated Show resolved Hide resolved
model-optimizer/extensions/ops/embedding_bag.py Outdated Show resolved Hide resolved
model-optimizer/extensions/middle/sparse_reshape.py Outdated Show resolved Hide resolved
@mvafin mvafin force-pushed the feature/embedding-bag-opset-3 branch from 7aface9 to 4da1a30 Compare June 5, 2020 12:32
mvafin and others added 2 commits June 5, 2020 18:04
@rkazants
Copy link
Contributor

rkazants commented Jun 8, 2020 •
edited
Loading

@lazarevevgeny, I have just checked e2e test for Wide&Deep and it needs modification by adding target_layer for eltwise comparator since IR has multiple extra output nodes due to Split nodes in IR. What is right way to to merge this PR first or update e2e test first? Please advice.

@rkazants
Fix clean-up for transformations
9b5e8de 
Signed-off-by: Roman Kazantsev <[email protected]>
@rkazants
Fix clean-up for transformation #2
954bc6f 
Signed-off-by: Roman Kazantsev <[email protected]>
@lazarevevgeny lazarevevgeny merged commit f1811ad into openvinotoolkit:master Jun 8, 2020
@mvafin mvafin deleted the feature/embedding-bag-opset-3 branch February 9, 2023 18:25
redradist pushed a commit to redradist/openvino that referenced this pull request Oct 6, 2023
@allnes @ilya-lavrenov
init (openvinotoolkit#546)
c9a60a4 
Co-authored-by: Ilya Lavrenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jane-intel jane-intel jane-intel approved these changes

@rkazants rkazants rkazants left review comments

@lazarevevgeny lazarevevgeny lazarevevgeny approved these changes

Assignees

@lazarevevgeny lazarevevgeny

Labels
category: MO Model Optimizer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvafin @rkazants @jane-intel @lazarevevgeny