Skip to content

[jit] move casting ops from prim to aten #21002

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
wanchaol wants to merge 3 commits into from
Closed

[jit] move casting ops from prim to aten #21002

wanchaol wants to merge 3 commits into from

Conversation

@wanchaol
Copy link
Collaborator

@wanchaol wanchaol commented May 28, 2019 •
edited
Loading

Stack from ghstack:

Summary:
Currently we are messed up with prim and aten namespace when we register prim and builtin ops, lots of the ops that have schema should go into the aten namespace rather than prim, prim namespace should only be reserved for the ops that does not have a schema.

Differential Revision: D15523444

@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels May 28, 2019
This was referenced May 28, 2019
Closed
Closed
@wanchaol wanchaol requested review from driazati, eellison, suo and zdevito May 28, 2019 18:57
driazati
driazati reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@driazati driazati left a comment

Choose a reason for hiding this comment

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

What's the backwards compatibility strategy for stuff like this? Models saved in 1.1 wouldn't work here, should we keep the old ones around in legacy_ops.cpp or something?

@wanchaol
Copy link
Collaborator Author

wanchaol commented May 28, 2019

What's the backwards compatibility strategy for stuff like this? Models saved in 1.1 wouldn't work here, should we keep the old ones around in legacy_ops.cpp or something?

@driazati fortunately, all of our casting ops are serializing as python syntax like float(), int(), etc. So this PR should be fine on backward compatability. For other ops like abs, there're indeed bc issues and I think we should probably make a more concrete design on deprecating them.

@wanchaol wanchaol mentioned this pull request May 28, 2019
Closed
wanchaol added 2 commits May 28, 2019 16:54
@wanchaol
Update on "[jit] move casting ops from prim to aten"
df9539c 
[jit] move casting ops from prim to aten

gh-metadata: pytorch pytorch 21002 gh/wanchaol/11/head
@wanchaol
resolve conflict on "[jit] move casting ops from prim to aten"
cb0cc0e 
[jit] move casting ops from prim to aten

gh-metadata: pytorch pytorch 21002 gh/wanchaol/11/head
@suo
Copy link
Member

suo commented May 29, 2019

Can you explain why we are making this change?

@wanchaol
Copy link
Collaborator Author

wanchaol commented May 29, 2019

Can you explain why we are making this change?

@suo sorry for not adding the summary, I will update. The reason of making the change is, currently we are messed up with prim and aten namespace when we register prim and builtin ops, lots of the ops that have schema should go into the aten namespace rather than prim, prim namespace should only be reserved for the ops that does not have a schema.

zdevito
zdevito approved these changes May 29, 2019
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

Yep, our goal should be that prim:: is only used for non-schematizable ops

@zou3519 zou3519 deleted the gh/wanchaol/11/head branch May 29, 2019 19:39
@ezyang
Copy link
Contributor

ezyang commented May 29, 2019

This looks like it crossed over a merge resulting in master failure:

May 29 20:18:59 /var/lib/jenkins/workspace/torch/csrc/jit/script/python_sugared_value.cpp: In member function 'virtual std::shared_ptr<torch::jit::script::SugaredValue> torch::jit::script::ModuleValue::attr(const torch::jit::SourceRange&, torch::jit::script::Function&, const string&)':
May 29 20:18:59 /var/lib/jenkins/workspace/torch/csrc/jit/script/python_sugared_value.cpp:262:41: error: 'Bool' is not a member of 'torch::jit::prim'
May 29 20:18:59      Value* the_bool = m.graph()->insert(prim::Bool, {the_tensor});
May 29 20:18:59                                          ^
May 29 20:18:59 /var/lib/jenkins/workspace/torch/csrc/jit/script/python_sugared_value.cpp:262:41: note: suggested alternatives:
May 29 20:18:59 In file included from /var/lib/jenkins/workspace/aten/src/ATen/core/ivalue_inl.h:7:0,
May 29 20:18:59                  from /var/lib/jenkins/workspace/aten/src/ATen/core/ivalue.h:409,
May 29 20:18:59                  from /var/lib/jenkins/workspace/torch/csrc/jit/pybind_utils.h:3,
May 29 20:18:59                  from /var/lib/jenkins/workspace/torch/csrc/jit/script/python_sugared_value.h:4,
May 29 20:18:59                  from /var/lib/jenkins/workspace/torch/csrc/jit/script/python_sugared_value.cpp:1:
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:81:11: note:   'c10::aten::Bool'
May 29 20:18:59    _(aten, Bool)                    \
May 29 20:18:59            ^
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:331:35: note: in definition of macro 'DEFINE_SYMBOL'
May 29 20:18:59    namespace ns { constexpr Symbol s(static_cast<unique_t>(_keys::ns##_##s)); }
May 29 20:18:59                                    ^
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:332:1: note: in expansion of macro 'FORALL_NS_SYMBOLS'
May 29 20:18:59  FORALL_NS_SYMBOLS(DEFINE_SYMBOL)
May 29 20:18:59  ^
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:81:11: note:   'c10::aten::Bool'
May 29 20:18:59    _(aten, Bool)                    \
May 29 20:18:59            ^
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:331:35: note: in definition of macro 'DEFINE_SYMBOL'
May 29 20:18:59    namespace ns { constexpr Symbol s(static_cast<unique_t>(_keys::ns##_##s)); }
May 29 20:18:59                                    ^
May 29 20:18:59 /var/lib/jenkins/workspace/aten/src/ATen/core/interned_strings.h:332:1: note: in expansion of macro 'FORALL_NS_SYMBOLS'
May 29 20:18:59  FORALL_NS_SYMBOLS(DEFINE_SYMBOL)

@wanchaol
Copy link
Collaborator Author

wanchaol commented May 29, 2019

@ezyang reverting in progress.

zdevito pushed a commit to zdevito/ATen that referenced this pull request May 29, 2019
@wanchaol @facebook-github-bot
move casting ops from prim to aten (#21002)
b5d6913 
Summary:
Pull Request resolved: pytorch/pytorch#21002
ghimport-source-id: 4c88a54a3ecb76c5ca3c2c328b749350860a166d

Differential Revision: D15523444

Pulled By: wanchaol

fbshipit-source-id: 642342bf1ccea83c88897bc023979a32ee01addf
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 29, 2019

@wanchaol merged this pull request in a0111aa.

@wanchaol wanchaol mentioned this pull request Jun 26, 2019
Closed
wanchaol added a commit that referenced this pull request Jun 26, 2019
@wanchaol
[jit][redo] move casting ops from prim to aten
687a6ca 
Summary:

This is a redo PR of #21002

Currently we are messed up with prim and aten namespace when we register prim and builtin ops, lots of the ops that have schema should go into the aten namespace rather than prim, prim namespace should only be reserved for the ops that does not have a schema.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
wanchaol added a commit that referenced this pull request Jun 28, 2019
@wanchaol
Update on "[jit][redo] move casting ops from prim to aten"
ce8ac74 
[jit][redo] move casting ops from prim to aten

Summary:

This is a redo PR of #21002

Currently we are messed up with prim and aten namespace when we register prim and builtin ops, lots of the ops that have schema should go into the aten namespace rather than prim, prim namespace should only be reserved for the ops that does not have a schema.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

gh-metadata: pytorch pytorch 22275 gh/wanchaol/27/head
wanchaol added a commit that referenced this pull request Jul 3, 2019
@wanchaol
rebase and address comments on "[jit][redo] move casting ops from pri…
6561378 
…m to aten"

[jit][redo] move casting ops from prim to aten

Summary:

This is a redo PR of #21002

Currently we are messed up with prim and aten namespace when we register prim and builtin ops, lots of the ops that have schema should go into the aten namespace rather than prim, prim namespace should only be reserved for the ops that does not have a schema.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

gh-metadata: pytorch pytorch 22275 gh/wanchaol/27/head
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

@suo suo Awaiting requested review from suo

@eellison eellison Awaiting requested review from eellison

+1 more reviewer

@driazati driazati driazati left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: cpp Related to C++ API 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.

9 participants

@wanchaol @suo @ezyang @facebook-github-bot @zdevito @driazati @pytorchbot @mruberry