[jit][script] Handling for py2/py3 division differences by zou3519 · Pull Request #11016 · pytorch/pytorch

zou3519 · 2018-08-29T20:27:41Z

In Python 2, use of / (regardless of int/float/Tensor) causes a compiler error if
from __future__ import division is not imported in the file.
The / operator is universally set to do "true" division for integers
Added a prim::FloorDiv operator because it is used in loop unrolling.

The error if users use '/' in python 2 without importing from future
occurs when building the JIT AST.

cc @apaszke @zdevito

torch/csrc/jit/register_prim_ops.cpp

zdevito · 2018-08-30T18:25:57Z

I think the fix should be:

In Python 2, use of / should cause a compiler error if from __future__ import division is not imported in the file.
The / operator is universally set to do "true" division for integers, meaning it returns a float.
(optional, for now) We add the // operator support to script.

test/expect/TestScript.test_loop_unroll_unused_counter.expect

zou3519 · 2018-08-31T14:31:02Z

This should be good for another review. I'm a little concerned about ONNX export though (I'm not sure if I should be) because of the following:

ints get exported as scalar tensors to ONNX
aten::div(int, int) gives a float now because / is truediv. However, aten::div(int scalar tensor, int scalar tensor) gives an int scalar tensor because of division semantics on tensors...

@jamesr66a should I be concerned?

facebook-github-bot

zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zou3519 · 2018-08-31T15:38:13Z

Had offline chat with @apaszke, I am going to fix the ONNX export

zdevito

This looks good -- I have some minor comments.

torch/csrc/jit/register_prim_ops.cpp

test/expect/TestScript.test_loop_unroll_unused_counter.expect

torch/jit/frontend.py

test/test_jit.py

test/expect/TestScript.test_loop_unroll_unused_counter.expect

facebook-github-bot

zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

addressed

torch/csrc/jit/passes/prepare_division_for_onnx.h

+// - aten::div(int, int) -> float is the python truediv operator. This doesn't
+//   exist in ONNX so we express it in terms of ONNX ops.
+//
+TORCH_API void PrepareDivisionForONNX(const std::shared_ptr<Graph>& graph);


facebook-github-bot

zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

torch/onnx/utils.py

    torch._C._jit_pass_lint(graph)

+    # onnx only supports tensors, but 1 / 2 = 0.5 and tensor(1) / tensor(2) = 0
+    torch._C._jit_pass_prepare_division_for_onnx(graph)


jamesr66a

Fixup the ONNX pass and add TORCH_API to Node::matches (to make windows CI pass) then this should be good to go

torch/csrc/jit/passes/onnx/prepare_division_for_onnx.cpp

+        Value* longtensor = subgraph->insertNode(subgraph->createNumToTensor(input))->output();
+        // FLOAT = 1
+        // https://github.com/onnx/onnx/blob/6bedd27b0307c9295039bd847895a27275160a98/onnx/onnx.in.proto#L282
+        Node* cast = subgraph->create(onnx::Cast, {longtensor})->i_(attr::to, 1);


ezyang · 2018-09-04T15:48:27Z

@pytorchbot retest this please

ezyang · 2018-09-04T16:21:17Z

@pytorchbot retest this please

zou3519 · 2018-09-04T17:51:48Z

@pytorchbot retest this please

addressed

zdevito · 2018-09-04T21:24:57Z

This still looks good. There are some expect file issues.

- In Python 2, use of `/` (regardless of int/float/Tensor) causes a compiler error if `from __future__ import division` is not imported in the file. - The / operator is universally set to do "true" division for integers - Added a `prim::FloorDiv` operator because it is used in loop unrolling. The error if users use '/' in python 2 without importing from __future__ occurs when building the JIT AST.

- s/prim::FloorDiv/aten::floordiv - Fix onnx export for true division on integers

facebook-github-bot

zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: - In Python 2, use of `/` (regardless of int/float/Tensor) causes a compiler error if `from __future__ import division` is not imported in the file. - The / operator is universally set to do "true" division for integers - Added a `prim::FloorDiv` operator because it is used in loop unrolling. The error if users use '/' in python 2 without importing from __future__ occurs when building the JIT AST. cc apaszke zdevito Pull Request resolved: pytorch#11016 Differential Revision: D9613527 Pulled By: zou3519 fbshipit-source-id: 0cebf44d5b8c92e203167733692ad33c4ec9dac6

zou3519 requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners August 29, 2018 20:27

zou3519 added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 29, 2018

apaszke previously requested changes Aug 30, 2018

View reviewed changes

torch/csrc/jit/register_prim_ops.cpp Outdated

This comment was marked as off-topic.

Sign in to view

zdevito reviewed Aug 30, 2018

View reviewed changes

torch/csrc/jit/register_prim_ops.cpp Outdated Show resolved Hide resolved

zou3519 force-pushed the div-py3 branch 2 times, most recently from fb3a86e to 12091d4 Compare August 31, 2018 14:26

zou3519 commented Aug 31, 2018

View reviewed changes

test/expect/TestScript.test_loop_unroll_unused_counter.expect Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot reviewed Aug 31, 2018

View reviewed changes

zdevito approved these changes Aug 31, 2018

View reviewed changes

apaszke reviewed Aug 31, 2018

View reviewed changes

test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

test/expect/TestScript.test_loop_unroll_unused_counter.expect Outdated

This comment was marked as off-topic.

Sign in to view

zou3519 force-pushed the div-py3 branch from 0da7cc7 to 0def65a Compare August 31, 2018 18:18

zou3519 requested review from Yangqing, anderspapitto, bddppq, dzhulgakov, houseroad, jamesr66a and smessmer as code owners August 31, 2018 18:18

facebook-github-bot reviewed Aug 31, 2018

View reviewed changes

apaszke reviewed Aug 31, 2018

View reviewed changes

facebook-github-bot reviewed Aug 31, 2018

View reviewed changes

zdevito reviewed Aug 31, 2018

View reviewed changes

jamesr66a previously requested changes Sep 1, 2018

View reviewed changes

zou3519 force-pushed the div-py3 branch 2 times, most recently from 95a42f2 to 943f4e7 Compare September 4, 2018 14:50

jamesr66a approved these changes Sep 4, 2018

View reviewed changes

zou3519 added 6 commits September 5, 2018 10:22

Forgot to add test files

a7b566c

Address comments:

ebba69c

- s/prim::FloorDiv/aten::floordiv - Fix onnx export for true division on integers

Simply __future__ division check

959fc35

Move prepare_division_for_onnx.cpp from passes/ to passes/onnx

16276ff

Address comments: use aten::_cast_Float instead of onnx::Cast

ac3a929

zou3519 force-pushed the div-py3 branch from 943f4e7 to ac3a929 Compare September 5, 2018 17:36

facebook-github-bot reviewed Sep 5, 2018

View reviewed changes

facebook-github-bot closed this in 68c2e01 Sep 5, 2018

zou3519 deleted the div-py3 branch September 6, 2018 14:37

ezyang added the merged label Jun 26, 2019

Conversation

zou3519 commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

zdevito commented Aug 30, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

zou3519 commented Aug 31, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zou3519 commented Aug 31, 2018

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

jamesr66a left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

ezyang commented Sep 4, 2018

Uh oh!

ezyang commented Sep 4, 2018

Uh oh!

zou3519 commented Sep 4, 2018

Uh oh!

zdevito commented Sep 4, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zou3519 commented Aug 29, 2018 •

edited

Loading