Skip to content

[ONNX] Update embedding_bag export #44693

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
Closed
neginraoof wants to merge 14 commits into from
Closed

[ONNX] Update embedding_bag export #44693

neginraoof wants to merge 14 commits into from

Conversation

@neginraoof
Copy link
Contributor

@neginraoof neginraoof commented Sep 15, 2020

Export of embedding bag with dynamic list of offsets.

@neginraoof neginraoof requested a review from apaszke as a code owner September 15, 2020 06:54
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 15, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 15, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 0e0392c (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 30 times.

BowenBao
BowenBao reviewed Sep 18, 2020
View reviewed changes
@codecov
Copy link

codecov bot commented Sep 18, 2020 •
edited
Loading

Codecov Report

Merging #44693 into master will decrease coverage by 0.04%.
The diff coverage is 12.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #44693      +/-   ##
==========================================
- Coverage   68.47%   68.43%   -0.05%     
==========================================
  Files         405      405              
  Lines       51821    51863      +42     
==========================================
+ Hits        35485    35492       +7     
- Misses      16336    16371      +35
Impacted Files Coverage Δ
torch/onnx/symbolic_opset11.py 21.42% <7.14%> (-1.28%) ⬇️
torch/onnx/symbolic_opset10.py 40.00% <16.66%> (+3.74%) ⬆️
torch/onnx/utils.py 70.56% <21.73%> (-1.51%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c8a600...0e0392c. Read the comment docs.

KsenijaS
KsenijaS reviewed Sep 19, 2020
View reviewed changes
@gchanan gchanan added this to the 1.7.0 milestone Sep 21, 2020
BowenBao
BowenBao approved these changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

LGTM, please rebase to pass unrelated test failures.

@neginraoof
Copy link
Contributor Author

neginraoof commented Sep 22, 2020

@bzinodev Can we merge this please?
Thanks!

facebook-github-bot
facebook-github-bot reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

KsenijaS
KsenijaS reviewed Sep 24, 2020
View reviewed changes
@neginraoof
Copy link
Contributor Author

neginraoof commented Sep 29, 2020

cc @bzinodev
This PR is rebased and ready for merge. Thanks a lot.

facebook-github-bot
facebook-github-bot reviewed Sep 30, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 1, 2020

@bzinodev merged this pull request in 6b42ca2.

neginraoof added a commit to neginraoof/pytorch that referenced this pull request Oct 2, 2020
@neginraoof
[ONNX] Update embedding_bag export (pytorch#44693)
ff95d07 
Summary:
Export of embedding bag with dynamic list of offsets.

Pull Request resolved: pytorch#44693

Reviewed By: malfet

Differential Revision: D23831980

Pulled By: bzinodev

fbshipit-source-id: 3eaff1a0f20d1bcfb8039e518d78c491be381e1a
This was referenced Oct 2, 2020
Merged
Closed
soumith pushed a commit that referenced this pull request Oct 6, 2020
@neginraoof
[ONNX] Update embedding_bag export (#44693) (#45756)
8f8da60 
Summary:
Export of embedding bag with dynamic list of offsets.

Pull Request resolved: #44693

Reviewed By: malfet

Differential Revision: D23831980

Pulled By: bzinodev

fbshipit-source-id: 3eaff1a0f20d1bcfb8039e518d78c491be381e1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@BowenBao BowenBao BowenBao approved these changes

@apaszke apaszke Awaiting requested review from apaszke

+2 more reviewers

@KsenijaS KsenijaS KsenijaS left review comments

@bzinodev bzinodev bzinodev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

8 participants

@neginraoof @facebook-github-bot @KsenijaS @BowenBao @bzinodev @gchanan @pytorchbot @mruberry