Support Python Slice class in TorchScript #44335

gmagogsfm · 2020-09-08T22:12:14Z

Implements support for Python Slice class (not slice expression, which is already supported)

Slice object can be used in any place that supports slice expression, including multi-dim tensor slicing.

Fixes #43511
Fixes #43125

dr-ci · 2020-09-08T22:18:02Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/caffe2-pytorch-linux-bionic-rocm3.7-py3.6-test

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 86 times.

gmagogsfm · 2020-09-10T01:16:42Z

torch/csrc/jit/frontend/ir_emitter.cpp

Note to reviewer:

I changed name of this argument from "begin" to "start" to match the schema of aten::slice. I think it worked before because this argument is always used as positional argument. Let me know if there are other things to consider.

SplitInfinity

What is the motivation for this work? What can slice objects support that slice expressions do not already support?

torch/csrc/jit/python/python_sugared_value.cpp

torch/csrc/jit/python/python_sugared_value.h

torch/csrc/jit/python/python_sugared_value.cpp

torch/csrc/jit/frontend/ir_emitter.cpp

SplitInfinity · 2020-09-10T19:37:06Z

torch/csrc/jit/frontend/ir_emitter.cpp

Consider adding a comment like the one above which explains that this code handles slice objects.

SplitInfinity · 2020-09-10T19:46:50Z

torch/csrc/jit/frontend/sugared_value.h

Maybe SliceValue because this is a regular SugaredValue?

This is actually something I want to get advice on. It is a vanilla SugaredValue only because ir_emitter needs it and ir_emitter can't depend on python_sugared_value. In reality, PythonSliceValue does represent a python object, thus the python prefix. Let me know what you think.

Hm, there is a ClassValue class that also extends SugaredValue that represents Python classes. So I think SliceValue would be better here.

test/jit/test_slice.py

gmagogsfm

As for motivation, this is to fix #43511

The only one case where a slice object is better than slice expression, that is to share slice expression among different objects. Otherwise they are equivalent. So motivation is to fix the issue and enable one more language feature to save users some work-around time.

test/jit/test_slice.py

gmagogsfm · 2020-09-10T20:28:29Z

torch/csrc/jit/frontend/ir_emitter.cpp

gmagogsfm · 2020-09-10T20:30:32Z

torch/csrc/jit/frontend/sugared_value.h

This is actually something I want to get advice on. It is a vanilla SugaredValue only because ir_emitter needs it and ir_emitter can't depend on python_sugared_value. In reality, PythonSliceValue does represent a python object, thus the python prefix. Let me know what you think.

torch/csrc/jit/python/python_sugared_value.cpp

torch/csrc/jit/python/python_sugared_value.h

codecov · 2020-09-11T01:20:11Z

Codecov Report

Merging #44335 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #44335   +/-   ##
=======================================
  Coverage   68.04%   68.04%           
=======================================
  Files         384      384           
  Lines       49814    49814           
=======================================
  Hits        33897    33897           
  Misses      15917    15917

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5cc151...dc08fe7. Read the comment docs.

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

SplitInfinity

Why did so few CI jobs run on this? Please try to get that extra signal before attempting to land this.

gmagogsfm · 2020-09-16T19:40:40Z

Why did so few CI jobs run on this? Please try to get that extra signal before attempting to land this.

Because I rebased this morning to circumvent a Phabricator patch issue. You happened to open this PR when I just rebased, so CIs are just beginning.

facebook-github-bot

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

facebook-github-bot · 2020-09-17T08:14:14Z

@gmagogsfm merged this pull request in 9909327.

Summary: Implements support for[ Python Slice class](https://docs.python.org/3/c-api/slice.html) (not slice expression, which is already supported) Slice object can be used in any place that supports slice expression, including multi-dim tensor slicing. Fixes #43511 Fixes #43125 Pull Request resolved: #44335 Reviewed By: suo, jamesr66a Differential Revision: D23682213 Pulled By: gmagogsfm fbshipit-source-id: f74fe25370e89fbfd2b3727d95ce4e1c4ba8dec4

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 8, 2020

gmagogsfm changed the title ~~Enable slice None handling~~ Support Python Slice class in TorchScript Sep 8, 2020

gmagogsfm commented Sep 10, 2020

View reviewed changes

gmagogsfm marked this pull request as ready for review September 10, 2020 02:10

gmagogsfm requested a review from apaszke as a code owner September 10, 2020 02:10

gmagogsfm requested review from SplitInfinity, eellison and wanchaol September 10, 2020 02:11

SplitInfinity reviewed Sep 10, 2020

View reviewed changes

test/jit/test_slice.py Outdated Show resolved Hide resolved

gmagogsfm commented Sep 10, 2020

View reviewed changes

facebook-github-bot reviewed Sep 14, 2020

View reviewed changes

facebook-github-bot reviewed Sep 16, 2020

View reviewed changes

SplitInfinity approved these changes Sep 16, 2020

View reviewed changes

Implement Python slice

dc08fe7

facebook-github-bot reviewed Sep 17, 2020

View reviewed changes

facebook-github-bot closed this in 9909327 Sep 17, 2020

facebook-github-bot added the merged label Sep 17, 2020

crisp-snakey mentioned this pull request Oct 4, 2020

Improve ONNX tracing support for Faster-RCNN facebookresearch/detectron2#2060

Closed

mruberry added the Merged label Oct 28, 2020

Support Python Slice class in TorchScript #44335

Support Python Slice class in TorchScript #44335

Uh oh!

Conversation

gmagogsfm commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmagogsfm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

gmagogsfm commented Sep 16, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

gmagogsfm commented Sep 8, 2020 •

edited

Loading

dr-ci bot commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 11, 2020 •

edited

Loading