Unify boxed function signature between jit and c10

[smessmer]

Stack from ghstack:

• Unify boxed function signature between jit and c10 #37034 Unify boxed function signature between jit and c10

c10 takes a Stack* in boxed functions while JIT took Stack&.
c10 doesn't return anything while JIT returns an int which is always zero.

This changes JIT to follow the c10 behavior.

Differential Revision: D20567950

[dr-ci]

💊 CI failures summary and remediations

As of commit 14ea12a (more details on the Dr. CI page):

---

• 1/2 failures introduced in this PR
• 1/2 broken upstream at merge base b35cdc5 since Jun 26

---

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_py3_6_gcc5_4_build (1/1)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

 
  git checkout -b <new-branch-name> 
 
HEAD is now at 14ea12a630 Update on "Unify boxed function signature between jit and c10" 
+ git reset --hard 14ea12a630d67c6641a4bf4870ba7186640f8a71 
HEAD is now at 14ea12a630 Update on "Unify boxed function signature between jit and c10" 
+ git merge --allow-unrelated-histories --no-edit --no-ff 0235676f8a3c7a6e64b250151faec21037bdd402 
Auto-merging torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp 
CONFLICT (content): Merge conflict in torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp 
Auto-merging torch/csrc/jit/runtime/register_prim_ops.cpp 
Automatic merge failed; fix conflicts and then commit the result. 

---

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:

• pytorch_xla_linux_bionic_py3_6_clang9_build since Jun 26
  • 🔁 rerun

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 59 times.

[bhosmer]

LGTM

[bhosmer]

Given the switch from Stack& to Stack*, why do we still need the old Stack& overloads? Will probably find out further down the PR, but leaving this here just in case not :P

[smessmer]

because otherwise we'd have to change even more code. They probably should die at some point though.

[bhosmer]

This change isn't strictly necessary, right?

[smessmer]

true, this is something I cleaned up on the way

[smessmer]

@bhosmer lol remember the msvc issue where we had to land exactly this to fix it? I should have landed this sooner and we would have had to do a lot less debugging there...

[bhosmer]

ugh yeah as you were describing the fix I was thinking of this change and wondering where it had gone 😝

[bddppq]

Our project has registered custom jit operators (outside of the pytorch tree), and this diff breaks them because the Operation signature has been changed.
Is it that directly registering jit operator considered jit internal details and so not much backward compatibility guarantee going forward? From quickly browsing some projects that adds custom op, they seem to directly register jit operator as well (and this change actually also breaks them)

• https://github.com/NVIDIA/TRTorch/blob/223d8ddf9301a3fcb68167757a3f997b9e0c7d45/core/execution/register_trt_op.cpp#L58
• https://github.com/pytorch/tvm/blob/9d3ca577133152b04820fdd283291df5aacc6415/torch_tvm/register.h#L38

[smessmer]

The torch::jit::RegisterOperators API is only meant for internal prim ops and not public API. The public operator registration API was torch::RegisterOperators and we just started building a new pybind-style one to replace torch::RegisterOperators (see documentation). Does one of those work for your use case? Note that you'd have to write your operators in an unboxed way, e.g. Tensor my_add(Tensor a, Tensor b) {...} instead of in a boxed way like void my_add(vector<IValue>* args).

[smessmer]

Btw, are you certain those projects break? I think TVM will likely break because they need access to the JIT node and that changed, but I think the TRTorch project shouldn't break. The way they're calling still works, just throws a deprecation warning now.

[bddppq]

The reason we chose to directly register jit operator is that our custom op's kernel needs to access the attribute of its associated jit node (similar to the torch_tvm use case linked in my last comment).

[bddppq]

Btw, are you certain those projects break? I think TVM will likely break because they need access to the JIT node and that changed, but I think the TRTorch project shouldn't break. The way they're calling still works, just throws a deprecation warning now.

I have not tried actually building them, maybe you are right that TRTorch doesn't break.

[smessmer]

hm yes, in that case you need torch::jit::RegisterOperators. Note that the whole jit Node class isn't part of the public API and could change. It's ok to use it but you'll likely have to adapt your code from time to time.