Skip to content

Deprecate untyped Dicts #22516

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
Parent: Add extra debug info for the super common "index out of bounds" error.
wants to merge 34 commits into from
Closed

Deprecate untyped Dicts #22516

wants to merge 34 commits into from

Conversation

@smessmer
Copy link
Contributor

@smessmer smessmer commented Jul 3, 2019 •
edited
Loading

Stack:
    :white_circle:  #22518 Close loophole to create untyped tuples  💛
    :white_circle:  #22517 Deprecate untyped Lists  💛
    :black_circle:  #22516 Deprecate untyped Dicts  💚

Force anybody creating an untyped Dict to call c10::impl::deprecatedUntypedDict().
This should hopefully make it clear that this is not public API and prevent people from using it.

Differential Revision: D16115215

smessmer added 30 commits June 19, 2019 18:10
@smessmer
V1: Initial commit
5ecdfc7 
Differential Revision: D15906387
Differential Version: 85235547
@smessmer
V1: Initial commit
77cca0d 
Differential Revision: D15914462
Differential Version: 85265428
@smessmer
V1: Merge with parent diff changes
8a2a23e 
Differential Revision: D15906387
Differential Version: 85316125
@smessmer
V1: Merge with parent diff changes
1068b35 
Differential Revision: D15914462
Differential Version: 85316120
@smessmer
V4: Merge with parent diff changes
c13efc6 
Differential Revision: D15906387
Differential Version: 85337731
@smessmer
V2: Merge with parent diff changes
b850bf3 
Differential Revision: D15914462
Differential Version: 85337732
@smessmer
V6: Merge with parent diff changes
64a6c01 
Differential Revision: D15906387
Differential Version: 85373861
@smessmer
V3: Merge with parent diff changes
384edfc 
Differential Revision: D15914462
Differential Version: 85373860
@smessmer
V1: Initial commit
30396f4 
Differential Revision: D15948098
Differential Version: 85395386
@smessmer
V7: Merge with parent diff changes
ab2f9d6 
Differential Revision: D15906387
Differential Version: 85398730
@smessmer
V4: Merge with parent diff changes
8a3f900 
Differential Revision: D15914462
Differential Version: 85401866
@smessmer
V2: Merge with parent diff changes
06b9aa7 
Differential Revision: D15948098
Differential Version: 85510078
@smessmer
V8: Merge with parent diff changes
ecb974c 
Differential Revision: D15906387
Differential Version: 85510075
@smessmer
V5: Merge with parent diff changes
b9fd4f4 
Differential Revision: D15914462
Differential Version: 85510076
@smessmer
V3: (no description)
26a427d 
Differential Revision: D15948098
Differential Version: 85520501
@smessmer
V9: Merge with parent diff changes
43e30a2 
Differential Revision: D15906387
Differential Version: 85520502
@smessmer
V6: Merge with parent diff changes
44dd10d 
Differential Revision: D15914462
Differential Version: 85520503
@smessmer
V4: (no description)
5b005d8 
Differential Revision: D15948098
Differential Version: 85559958
@smessmer
V10: Merge with parent diff changes
8e60a62 
Differential Revision: D15906387
Differential Version: 85559959
@smessmer
V7: Merge with parent diff changes
e57d127 
Differential Revision: D15914462
Differential Version: 85559961
@smessmer
V11: Merge with parent diff changes
2a6d11a 
Differential Revision: D15906387
Differential Version: 85650910
@smessmer
V8: Merge with parent diff changes
4cda83b 
Differential Revision: D15914462
Differential Version: 85650911
@smessmer
V12: Merge with parent diff changes
65fee0b 
Differential Revision: D15906387
Differential Version: 85727283
@smessmer
V9: Merge with parent diff changes
f784ea3 
Differential Revision: D15914462
Differential Version: 85727285
@smessmer
V10: Merge with parent diff changes
352e3c3 
Differential Revision: D15914462
Differential Version: 86027332
@smessmer
V11: (no description)
42c3ac9 
Differential Revision: D15914462
Differential Version: 86030563
@smessmer
V12: (no description)
8b47d94 
Differential Revision: D15914462
Differential Version: 86030907
@smessmer
V13: (no description)
a821077 
Differential Revision: D15914462
Differential Version: 86031209
@smessmer
V14: (no description)
e6f760c 
Differential Revision: D15914462
Differential Version: 86035561
@smessmer
V1: Initial commit
d107bc0 
Differential Revision: D16115215
Differential Version: 86035562
@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: internals Related to internal abstractions in c10 and ATen labels Jul 3, 2019
This was referenced Jul 3, 2019
Closed
Closed
Closed
smessmer added 2 commits July 3, 2019 18:53
@smessmer
V15: (no description)
03e2783 
Differential Revision: D15914462
Differential Version: 86043406
@smessmer
V2: Merge with parent diff changes
d839f7a 
Differential Revision: D16115215
Differential Version: 86043409
@smessmer smessmer requested review from dzhulgakov and zdevito July 4, 2019 06:11
@smessmer
V3: (no description)
0caefdc 
Differential Revision: D16115215
Differential Version: 86049340
dzhulgakov
dzhulgakov approved these changes Jul 5, 2019
View reviewed changes
Copy link
Collaborator

@dzhulgakov dzhulgakov left a comment

Choose a reason for hiding this comment

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

This is ok by itself, but what do you plan to do with pickler/interpreter instructions?

Also we need to be BC, so probably there are models already with the dict construct instruction

@smessmer
Copy link
Contributor Author

smessmer commented Jul 5, 2019

torch::Dict wasn't part of PyTorch 1.1, that's why I want to change this before we release 1.2. I haven't looked too closely at how to fix these call sites yet. The jit ops can maybe look at the graph to figure out the type. Pickler, not sure. Do we have to serialize the type? If yes, that's probably something we should also change before the 1.2 release.

@smessmer
V6: Merge with parent diff changes
f532e5b 
Differential Revision: D16115215
Differential Version: 86101224
@smessmer smessmer changed the base branch from export-D15914462 to master July 5, 2019 23:58
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in a09150a.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 6, 2019
@smessmer @facebook-github-bot
Deprecate untyped Dicts (#22516)
a1244bc 
Summary:
Pull Request resolved: pytorch/pytorch#22516

Force anybody creating an untyped Dict to call c10::impl::deprecatedUntypedDict().
This should hopefully make it clear that this is not public API and prevent people from using it.

Differential Revision: D16115215

fbshipit-source-id: 2ef4cb443da1cdf4ebf5b99851f69de0be730b97
@dzhulgakov
Copy link
Collaborator

We do serialize the types of attributes, but it's in a slightly different part of the code from where the dict gets created

@ezyang ezyang deleted the export-D16115215 branch July 19, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dzhulgakov dzhulgakov dzhulgakov approved these changes

@zdevito zdevito Awaiting requested review from zdevito

Assignees

No one assigned

Labels

Merged module: internals Related to internal abstractions in c10 and ATen 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.

5 participants

@smessmer @facebook-github-bot @dzhulgakov @pytorchbot @mruberry