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

[jit] Fix dict type serialization #32569

Closed
wants to merge 7 commits into from
Closed

[jit] Fix dict type serialization #32569

wants to merge 7 commits into from

Conversation

suo
Copy link
Member

@suo suo commented Jan 24, 2020 •
edited
Loading

Stack from ghstack:

If the dict's contained types cannot be inferred from its contents (for
example, Dict[str, Tensor] vs. Dict[str, Optional[Tensor]]), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Differential Revision: D19551016

@suo
[jit] Fix dict type serialization
53cfc90 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

[ghstack-poisoned]
@suo suo requested a review from apaszke as a code owner January 24, 2020 02:32
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jan 24, 2020
@suo
Update on "[jit] Fix dict type serialization"
1a311ad 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

[ghstack-poisoned]
@suo suo requested review from zdevito, chauhang and eellison and removed request for chauhang January 24, 2020 02:35
@suo
Update on "[jit] Fix dict type serialization"
b2acdb0 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

[ghstack-poisoned]
@suo
Update on "[jit] Fix dict type serialization"
a36035a 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Differential Revision: [D19551016](https://our.internmc.facebook.com/intern/diff/D19551016)

[ghstack-poisoned]
zdevito
zdevito approved these changes Jan 24, 2020
View reviewed changes
torch/csrc/jit/passes/python_print.cpp Outdated
if (node->inputs().size() == 0 && !is_default_type) {
// Empty dicts must be annotated with their type so the compiler knows
// what type is supposed to be inside them
if (node->inputs().size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This branch can be folded into the one below as another or clause. printDict knows know to print an empty Dict.

@suo
Update on "[jit] Fix dict type serialization"
c7755cc 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Differential Revision: [D19551016](https://our.internmc.facebook.com/intern/diff/D19551016)

[ghstack-poisoned]
suo added a commit that referenced this pull request Jan 24, 2020
@suo
[jit] Fix dict type serialization
396b66e 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

ghstack-source-id: eb4745c6d69abe97e0fade9df20ac3f6c9783079
Pull Request resolved: #32569
@suo
Update on "[jit] Fix dict type serialization"
29287c2 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Differential Revision: [D19551016](https://our.internmc.facebook.com/intern/diff/D19551016)

[ghstack-poisoned]
@suo
Update on "[jit] Fix dict type serialization"
9fde091 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Differential Revision: [D19551016](https://our.internmc.facebook.com/intern/diff/D19551016)

[ghstack-poisoned]
suo added a commit that referenced this pull request Jan 24, 2020
@suo
[jit] Fix dict type serialization
a163b5e 
If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

ghstack-source-id: fa0f869ebd5d51b9ab401d45a165bf540767d9bb
Pull Request resolved: #32569
@facebook-github-bot
Copy link
Contributor

@suo merged this pull request in 8fd3eae.

@facebook-github-bot facebook-github-bot deleted the gh/suo/255/head branch January 27, 2020 15:18
@cndn cndn mentioned this pull request Jan 27, 2020
Closed
wuhuikx pushed a commit to wuhuikx/pytorch that referenced this pull request Jan 30, 2020
@suo @facebook-github-bot
[jit] Fix dict type serialization (pytorch#32569)
2082550 
Summary:
Pull Request resolved: pytorch#32569

If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Test Plan: Imported from OSS

Differential Revision: D19551016

Pulled By: suo

fbshipit-source-id: c529b112e72c10f509a6bc0f5876644caa1be967
cndn added a commit to cndn/fairseq that referenced this pull request Jan 30, 2020
@cndn @facebook-github-bot
Add save and load tests to fairseq export test (facebookresearch#1653)
be34a59 
Summary:
Pull Request resolved: facebookresearch#1653

Earlier we had some issues at pickling. Type information gets lost. Fixed in pytorch/pytorch#32569.

These save_and_load tests are added for protection in the future.

Reviewed By: myleott

Differential Revision: D19435988

fbshipit-source-id: 76f31c78fd732c0e6e3a87bf1e16f5a9a0553d80
facebook-github-bot pushed a commit to facebookresearch/fairseq that referenced this pull request Jan 31, 2020
@cndn @facebook-github-bot
Add save and load tests to fairseq export test (#1653)
3df10a9 
Summary:
Pull Request resolved: #1653

Earlier we had some issues at pickling. Type information gets lost. Fixed in pytorch/pytorch#32569.

These save_and_load tests are added for protection in the future.

Reviewed By: myleott

Differential Revision: D19435988

fbshipit-source-id: 560ea65ed3493bebcf394327818364b3fcd6fc92
ttumiel pushed a commit to ttumiel/pytorch that referenced this pull request Mar 4, 2020
@suo @ttumiel
[jit] Fix dict type serialization (pytorch#32569)
f1bf10f 
Summary:
Pull Request resolved: pytorch#32569

If the dict's contained types cannot be inferred from its contents (for
example, `Dict[str, Tensor]` vs. `Dict[str, Optional[Tensor]]`), we must
explicitly annotate the type.

Also this removes some special handling that omits annotations on empty
containers that have the default type. It makes the code more complex
for not too much value, and was wrong for dicts anyway.

Test Plan: Imported from OSS

Differential Revision: D19551016

Pulled By: suo

fbshipit-source-id: c529b112e72c10f509a6bc0f5876644caa1be967
moussaKam pushed a commit to moussaKam/language-adaptive-pretraining that referenced this pull request Sep 29, 2020
@cndn @facebook-github-bot
Add save and load tests to fairseq export test (facebookresearch#1653)
55f2c07 
Summary:
Pull Request resolved: facebookresearch#1653

Earlier we had some issues at pickling. Type information gets lost. Fixed in pytorch/pytorch#32569.

These save_and_load tests are added for protection in the future.

Reviewed By: myleott

Differential Revision: D19435988

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

@zdevito zdevito zdevito approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@eellison eellison Awaiting requested review from eellison

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.
4 participants
@suo @facebook-github-bot @zdevito @mruberry