Skip to content

Fix incorrect EnumValue serialization issue #44891

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
gmagogsfm wants to merge 1 commit into from
Closed

Fix incorrect EnumValue serialization issue #44891

gmagogsfm wants to merge 1 commit into from

Conversation

@gmagogsfm
Copy link
Contributor

@gmagogsfm gmagogsfm commented Sep 17, 2020 •
edited
Loading

Previously, prim::EnumValue is serialized to ops.prim.EnumValue, which doesn't have the right implementation to refine return type. This diff correctly serializes it to enum.value, thus fixing the issue.

Fixes #44892

@gmagogsfm gmagogsfm mentioned this pull request Sep 17, 2020
Closed
@dr-ci
Copy link

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

💊 CI failures summary and remediations

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

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

ci.pytorch.org: 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 18 times.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 17, 2020
@gmagogsfm gmagogsfm marked this pull request as ready for review September 20, 2020 20:00
@gmagogsfm gmagogsfm requested a review from apaszke as a code owner September 20, 2020 20:00
@gmagogsfm gmagogsfm changed the title Repro to string enum type issue Fix incorrect EnumValue serialization issue Sep 21, 2020
eellison
eellison approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

lgtm

torch/csrc/jit/serialization/python_print.cpp Outdated
Copy link
Contributor

@eellison eellison Sep 21, 2020

Choose a reason for hiding this comment

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

Maybe add a comment here that these ops have to be resolved by the compiler instead of by schema matching for return type to be set appropriately

Copy link
Contributor Author

@gmagogsfm gmagogsfm Sep 21, 2020

Choose a reason for hiding this comment

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

Done.

facebook-github-bot
facebook-github-bot reviewed Sep 21, 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.

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

@codecov
Copy link

codecov bot commented Sep 21, 2020 •
edited
Loading

Codecov Report

Merging #44891 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #44891   +/-   ##
=======================================
  Coverage   67.85%   67.85%           
=======================================
  Files         384      384           
  Lines       50020    50020           
=======================================
+ Hits        33940    33941    +1     
+ Misses      16080    16079    -1
Impacted Files Coverage Δ
torch/testing/_internal/expecttest.py 77.55% <0.00%> (-1.03%) ⬇️
torch/utils/_benchmark/utils/common.py 79.33% <0.00%> (+1.65%) ⬆️

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 acc2a1e...e6be531. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 22, 2020

@gmagogsfm merged this pull request in c253b10.

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

@eellison eellison eellison approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@SplitInfinity SplitInfinity Awaiting requested review from SplitInfinity

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enum value's type in incorrectly inferred after save/load

4 participants

@gmagogsfm @facebook-github-bot @eellison @mruberry