Skip to content

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

Closed
wanchaol wants to merge 3 commits into
gh/wanchaol/11/basefrom
gh/wanchaol/11/head
Closed

[jit] move casting ops from prim to aten#21002
wanchaol wants to merge 3 commits into
gh/wanchaol/11/basefrom
gh/wanchaol/11/head

Conversation

@wanchaol

@wanchaol wanchaol commented May 28, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@driazati driazati left a comment

Copy link
Copy Markdown
Contributor

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

wanchaol commented May 28, 2019

Copy link
Copy Markdown
Collaborator Author

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

suo commented May 29, 2019

Copy link
Copy Markdown
Member

Can you explain why we are making this change?

@wanchaol

wanchaol commented May 29, 2019

Copy link
Copy Markdown
Collaborator Author

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

@zdevito zdevito left a comment

Copy link
Copy Markdown
Contributor

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

ezyang commented May 29, 2019

Copy link
Copy Markdown
Contributor

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

wanchaol commented May 29, 2019

Copy link
Copy Markdown
Collaborator Author

@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

facebook-github-bot commented May 29, 2019

Copy link
Copy Markdown
Contributor

@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
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@wanchaol @facebook-github-bot
move casting ops from prim to aten (pytorch#21002)
3c5b49a 
Summary:
Pull Request resolved: pytorch#21002
ghimport-source-id: 4c88a54

Differential Revision: D15523444

Pulled By: wanchaol

fbshipit-source-id: 642342bf1ccea83c88897bc023979a32ee01addf
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.

8 participants

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