[JIT] remove builtin interpolate functions #34514
Conversation
facebook-github-bot
added
the
oncall: jit
💊 CircleCI build failures summary and remediationsAs of commit fd82d4e (more details on the Dr. CI page):
🕵️ 2 new failures recognized by patternsThe following build failures do not appear to be due to upstream breakages (reran 2 jobs to discount flakiness):
|
test/test_jit.py
Outdated
| ('interpolate', torch.zeros(3, 3, 3).view(1, 1, 3, 3, 3), (2,), 'trilinear_5d', (True, 'aten::__interpolate')), | ||
| ('interpolate', torch.randn(S, M, M, M, M), (None, 2.), 'trilinear_5d_with_scale', (True, 'aten::__interpolate')), | ||
| ('interpolate', torch.randn(S, M, M, M, M), (4,), 'trilinear_5d_with_size', (True, 'aten::__interpolate')), | ||
| ('interpolate', torch.zeros(3, 3).view(1, 1, 3, 3), (2,), 'nearest_4d', (False, 'aten::__interpolate')), |
There was a problem hiding this comment.
What is the value that changed here and why
There was a problem hiding this comment.
per @ailzhang can be removed
| if size is None and scale_factor is None: | ||
| raise ValueError('either size or scale_factor should be defined') | ||
| if size is not None and scale_factor is not None: | ||
| raise ValueError('only one of size or scale_factor should be defined') |
There was a problem hiding this comment.
What happened to this error message? We should try to preserve the original behavior as much as possible
There was a problem hiding this comment.
i thought they were saying the same thing, but i guess they're slightly different. readded
| # type: (int, Tuple[Tensor, Optional[List[int]], Optional[float], Optional[bool]]) -> List[int] | ||
| pass | ||
|
|
||
| def _interp_output_size(dim, closed_over_args): # noqa: F811 |
There was a problem hiding this comment.
This function is kind of long, can you add an overload for _check_size_scale_factor as well?
There was a problem hiding this comment.
Because of overload decls, i would have to add 16 lines to separate out these 5 error checking lines, i don't think it's worth it, can do it if you think that's worth it.
test/test_jit.py
Outdated
| ('interpolate', torch.randn(S, M, M, M, M), (None, 2.), 'trilinear_5d_with_scale', (True, 'aten::__interpolate')), | ||
| ('interpolate', torch.randn(S, M, M, M, M), (4,), 'trilinear_5d_with_size', (True, 'aten::__interpolate')), | ||
| ('interpolate', torch.zeros(3, 3).view(1, 1, 3, 3), (2,), 'nearest_4d', (False, 'aten::__interpolate')), | ||
| ('interpolate', torch.randn(S, S, M, M), (None, 2.), 'nearest_4d_with_scale', (False, 'aten::__interpolate')), |
There was a problem hiding this comment.
False is the default value and it no longer need to check nodes in the differentiated graph. You can safely remove the last tuple (False, 'aten::__interpolate') for all these lines.
| // TODO: sort returns a tuple of Tensors, we have | ||
| // to extend the API to support that | ||
| // "sort", | ||
| "__interpolate", |
There was a problem hiding this comment.
could you add the corresponding ops to this list? I think it might be better to just put this change in the same PR
There was a problem hiding this comment.
I don't know the difference between "single_input_call_funcs" and "single_input_aten_funcs"
There was a problem hiding this comment.
Where should I be putting the ops?
|
"single_input_call_funcs" means functions compiled to CallFunction(xxx) like functional linear, single_input_aten_funcs means functions compiled to aten::xxx, like Conv2d
Sent from my iPhone
On Mar 11, 2020, at 10:02, eellison <notifications@github.com> wrote:
@eellison commented on this pull request.
________________________________
In torch/csrc/jit/passes/quantization.cpp<#34514 (comment)>:
@@ -136,20 +136,16 @@ bool isFunctionNode(Node* n,
// the quantization parameters for output given inputs
std::vector<size_t> getGeneralOpTensorInputIndexes(Node* n) {
std::vector<std::string> single_input_aten_funcs = {
- "adaptive_avg_pool2d",
- "max_pool2d",
- "avg_pool2d",
- "flatten",
- "max",
- "min",
- "mean",
- // TODO: sort returns a tuple of Tensors, we have
- // to extend the API to support that
- // "sort",
- "__interpolate",
Where should I be putting the ops?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#34514 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABF2R2M2EYH7TETBNCNKQCLRG67YNANCNFSM4LETHMDQ>.
|
|
I suppose they are compiled to aten ops, if that’s the case we should put them in the same list as previous __interpolate
Sent from my iPhone
On Mar 11, 2020, at 10:02, eellison <notifications@github.com> wrote:
@eellison commented on this pull request.
________________________________
In torch/csrc/jit/passes/quantization.cpp<#34514 (comment)>:
@@ -136,20 +136,16 @@ bool isFunctionNode(Node* n,
// the quantization parameters for output given inputs
std::vector<size_t> getGeneralOpTensorInputIndexes(Node* n) {
std::vector<std::string> single_input_aten_funcs = {
- "adaptive_avg_pool2d",
- "max_pool2d",
- "avg_pool2d",
- "flatten",
- "max",
- "min",
- "mean",
- // TODO: sort returns a tuple of Tensors, we have
- // to extend the API to support that
- // "sort",
- "__interpolate",
Where should I be putting the ops?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#34514 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABF2R2M2EYH7TETBNCNKQCLRG67YNANCNFSM4LETHMDQ>.
|
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Hi @eellison, I'm trying to look into changes breaking onnx tests. @torch.jit.script_method Is now producing a very large ir graph. |
Yes this is expected. We previously hacked it in as a builtin node, and are now representing it as its python code. In the short term it may be marginally slower, but in the long term will be faster, as we do a better job of optimizing away the non-Tensor ops, and potentially do codegen for the aten ops it invokes. It will also be more maintanable as now tracing & scripting creates the same ops, and we do not need 4 different implementations of interpolate (register_prim_ops, functional.py, symbolic_script, onnx_export -> functional.py). In your example mode will pretty much always be known at compile time. |
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@eellison |
It's not just a question of perf, but of maintainability. I've talked with other members of TorchScript team and vetted this change as it pertains to scripting. |
|
@eellison Thanks. |
|
@neginraoof it doesn't affect interrpolate tracing, which is vast majority of ONNX usage. I also said at the time interpolate script onnx export was implemented that we were going to remove "aten::__interpolate", and suggested that it be a requisite. If there is serious concerns about |
|
@eellison Do you know that is the future optimization plan? Are we going to have codegen for aten ops? Or is there another component optimizing the torch ir graph? I'm trying to understand whether this could be used to improve the ONNX graph as well. |
As it stands the plan is just to more aggressively optimize python idioms. I have a WIP pr that gets interpolate down to a single executed op. If someone thought it was worth the effort they could move "interpolate" to be a native aten function, but no one up to this point has thought it worth the cost/gain tradeoff. |
|
For anyone investigating breakage, I opened up disabled test here: #34658 |
| .check("aten::max") \ | ||
| .check("aten::min") \ | ||
| .check("aten::mean") \ | ||
| .check("aten::__interpolate") \ |
There was a problem hiding this comment.
@eellison we need the tests as well, actually I think this might have broken these tests, will sync with you tomorrow.
…erpolate (#35744) Summary: Since aten;:__interpolate is removed in #34514, we need a pass replace interpolate function with aten::__interpolate for ONNX export. Pull Request resolved: #35744 Reviewed By: hl475 Differential Revision: D20907041 Pulled By: houseroad fbshipit-source-id: f2d2cdfec47389245c50f538267124eedf682adf
7 participants
torch.nn.functional.interpolatewas written as a builtin op when we scripted the standard library, because it has four possible overloads. As a result, whenever we make a change tointerpolate, we need to make changes in two places, and it also makes it impossible to optimize the interpolate op. The builtin is tech debt.I talked with @ailzhang, and the symbolic script changes are good to remove (i guess that makes a third place we needed to re-implement interpolate).
I'm trying to get rid of unneccessary builtin operators because we're standardizing mobile bytecode soon, so we should try to get this landed as soon as possible.