Skip to content

[jit Python None should have its type inferred as NoneType #26665

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
suo wants to merge 17 commits into from
Closed

[jit Python None should have its type inferred as NoneType #26665

suo wants to merge 17 commits into from

Conversation

@suo
Copy link
Member

@suo suo commented Sep 23, 2019 •
edited
Loading

Stack from ghstack:

This is actually useful. For example: in batchnorm.py, all the tracked
stats are either nn.Parameter or None. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: D17551197

@suo
[jit Python None should have its type inferred as NoneType
9bc499c 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.
@suo suo requested a review from apaszke as a code owner September 23, 2019 20:55
@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 23, 2019
@suo suo mentioned this pull request Sep 23, 2019
Closed
9 tasks
resistor
resistor suggested changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@resistor resistor left a comment

Choose a reason for hiding this comment

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

Change LGTM, but testcase plz.

@suo
Update on "[jit Python None should have its type inferred as NoneType"
7d8544f 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.
@eellison
Copy link
Contributor

eellison commented Sep 24, 2019

I think this might have a bad interaction with the is None check partial compilation

@suo
Copy link
Member Author

suo commented Sep 24, 2019

I think this might have a bad interaction with the is None check partial compilation

Can you elaborate?

@eellison
Copy link
Contributor

eellison commented Sep 24, 2019

I think this might have a bad interaction with the is None check partial compilation

Can you elaborate?

Anytime there's a None attribute you can reasonably assume this is an Optional attribute. Bc of the partial compilation of if statements with is None checks, the optional behavior will get compiled away here if we infer None. This is valid for constants, but i'm not sure about generally assuming this to be true.

@suo
Update on "[jit Python None should have its type inferred as NoneType"
81eaa3b 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
suo added a commit that referenced this pull request Sep 25, 2019
@suo
[jit Python None should have its type inferred as NoneType
d2f579b 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

ghstack-source-id: 48b774f
Pull Request resolved: #26665
suo added 4 commits September 26, 2019 15:51
@suo
Update on "[jit Python None should have its type inferred as NoneType"
9ceb70a 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
08d2b20 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
65668ee 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
b6dc1a7 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@pytorchbot pytorchbot added caffe2 module: cpp Related to C++ API module: cuda Related to torch.cuda, and CUDA support in general module: internals Related to internal abstractions in c10 and ATen module: operators labels Oct 2, 2019
suo added 2 commits October 2, 2019 14:22
@suo
Update on "[jit Python None should have its type inferred as NoneType"
dd051dd 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
7ece65c 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo suo mentioned this pull request Oct 4, 2019
Closed
@suo
Update on "[jit Python None should have its type inferred as NoneType"
b53e52c 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
This was referenced Oct 4, 2019
Closed
Closed
suo added 6 commits October 6, 2019 01:11
@suo
Update on "[jit Python None should have its type inferred as NoneType"
2fc09d9 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
e1c99fa 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
0d437a7 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
b66d880 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
f4effac 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo
Update on "[jit Python None should have its type inferred as NoneType"
be6a893 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo suo requested a review from zdevito October 11, 2019 17:50
@suo suo mentioned this pull request Oct 11, 2019
Closed
@suo
Update on "[jit Python None should have its type inferred as NoneType"
2aa9c58 
This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Differential Revision: [D17551197](https://our.internmc.facebook.com/intern/diff/D17551197)
@suo suo mentioned this pull request Oct 12, 2019
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 12, 2019

@suo merged this pull request in 759c99c.

@facebook-github-bot facebook-github-bot deleted the gh/suo/173/head branch October 28, 2019 22:20
thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@suo
[jit Python None should have its type inferred as NoneType (pytorch#2…
614e4c3 
…6665)

Summary:
Pull Request resolved: pytorch#26665

This is actually useful. For example: in batchnorm.py, all the tracked
stats are either `nn.Parameter` or `None`. We should register them as
params if they are set, or attributes with type NoneType if they are
not.

Test Plan: Imported from OSS

Reviewed By: shannonzhu

Differential Revision: D17551197

Pulled By: suo

fbshipit-source-id: 8d6f6d76d4dab0d524c4ffdfe0c1dd465771cd00
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

@ebetica ebetica Awaiting requested review from ebetica

@goldsborough goldsborough Awaiting requested review from goldsborough

@yf225 yf225 Awaiting requested review from yf225

+1 more reviewer

@resistor resistor resistor requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

caffe2 Merged module: cpp Related to C++ API module: cuda Related to torch.cuda, and CUDA support in general module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries 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.

8 participants

@suo @eellison @facebook-github-bot @resistor @zdevito @pytorchbot @mruberry