You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/01/23 02:59:12 UTC
[GitHub] [incubator-tvm] masahi opened a new pull request #4769: [TOPI]
Remove cpp upsampling and resize op
masahi opened a new pull request #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769
Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577656902
I don't think C++ only solution is realistic so I prefer to keep only python. C++ resize in particular is hopelessly outdated and I don't want to be the one porting new functionalities from python. Even if I did that it wouldn't make my life any better.
People develop new stuff in python and it is not reasonable to ask them to port it to C++ when they send their PR. And after the python PR it is unlikely they would spend their time for C++ porting. Me and @jwfromm have been involved in development and review of resize op and we know what we are talking about :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577921569
We can have another discussion thread about the scope of ops that goes to cxx.
My current take is that the decision will likely be made as per group basis, eg all numpy ops could goes to c++ as numpy interface is pretty stable.
The same rule applies to schedule, so if a compute is stable but schedule is not, we can keep the schedule in py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577882904
Choosing cpp vs python per operator (rather than one lang for all ops) is a good compromise, but it creates other problems (need to discuss which impl to keep for each op, not obvious to newcomers which lang a particular op is implemented in, etc). But we can talk about that later.
So, going back to this PR, I propose to remove cpp resize and upsampling because,
* resize op is highly unstable. Recently people added new functionalities to resize python to support ONNX or Tensorflow use cases. ONNX, the one I'm familiar with, has a dozen of options that we have no support for. It is likely that we need to update our resize definition to cover more new or esoteric use cases from higher level frameworks / standards. resize cpp hasn't got any update and it can only do nearest/bilinear resize.
* upsampling cpp is useless and it simply forwards to resize cpp. If we remove resize cpp, it is automatically removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577944754
I am half joking but who knows, maybe we will get there
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577870143
I do agree that the cpp solution is desirable and has many benefits. For operators whose api and implementation would not be expected to change (pooling, elemwise op etc) it makes sense to have them in cpp only (and that is what we have currently). I also liked the recent effort which brought most of the relay build pipeline to cpp. A few days ago I sent a [PR](https://github.com/apache/incubator-tvm/pull/4751) which exposed some relay cpp functionality to python. Now that things are in cpp, this "exposing" can be done for any language (at least in principle).
C++ on topi is a different story :) I believe the cpp only solution is simply not realistic because of "human problem". We always say cpp is something "to strive for" or "to port over in the future", but it's never been a hard requirement. Also it is not clear what makes API "stable enough" to deserve a cpp impl. As soon as people find a reason not to do cpp, the cpp impl never happens. Most people who add features to topi just want to get their immediate jobs done, so "implement it in cpp" is usually not high on their list.
This ambiguity has created situations that our cpp resize op is in. In addition to the "outdated cpp" problem, there are many topi operators that only exist in python, including
* all sparse ops
* most of the vision specific ops (rcnn, ssd, deformable conv) except for the reorg op (for yolo)
* int8 and other lower bit ops
* framework specific ops (depth to space, fifo buffer etc)
and I'm sure there are others.
In summary,
* I agree that the cpp only solution is desirable and something we should strive for, but in practice not realistic
* Already too many inconsistencies on python vs cpp in topi. Most likely porting effort would not happen.
* The python only solution is the only viable option and basically it is already our status quo. We can keep the existing cpp impls that are useful, but we should make our python impl as the official one. This is also in line with the recent "Unified IR" initiative by @tqchen which, if I remember correctly, brings TVM to be more "Python first".
I usually don't have a strong opinion on things, but I believe this topi python vs cpp duplication and inconsistency situation is the most ugly corner in our code base and so wanted to say something about it. I'd love to hear other opinions :) @jwfromm @yzhliu @icemelon9
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577930418
so tvm will finally compile itself :) didn't know self hosting is one of our goal
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577874461
Hmm, i hear you. I think the nature of “need to adding new features” marks the op unstable, thus merit a python version as the primary one.
On the other hand, for most of the numpy style ops, whose interface are stable, it makes sense to encourage c++ version and use that as default(while removing the python version)
In both cases, for each op there is one primary language for the op, depending on the status. And we can remove the other one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577882904
Choosing cpp vs python per operator (rather than one lang for all ops) is a good compromise, but it creates other problems (need to discuss which impl to keep for each op, not obvious to newcomers which lang a particular op is implemented in, etc). But we can talk about that later.
So, going back to this PR, I propose to remove cpp resize and upsampling because,
* resize op is highly unstable. Recently people added new functionalities to resize python to support ONNX or Tensorflow use cases. ONNX, the one I'm familiar with, has a dozen of options that we have no support for. It is likely that we need to update our resize definition to cover more new or esoteric use cases from higher level frameworks / standards. resize cpp haven't got any update and it can only do nearest/bilinear resize.
* upsampling cpp is useless and it simply forwards to resize cpp. If we remove resize cpp, it is automatically removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577882904
Choosing cpp vs python per operator (rather than one lang for all ops) is a good compromise, but it creates other problems (need to discuss which impl to keep for each op, not obvious to newcomers which lang a particular op is implemented in, etc). But we can talk about that later.
So for this PR, I propose to remove cpp resize and upsampling because,
* resize op is highly unstable. Recently people added new functionalities to resize python to support ONNX or Tensorflow use cases. ONNX, the one I'm familiar with, has a dozen of options that we have no support for. It is likely that we need to update our resize definition to cover more new or esoteric use cases from higher level frameworks / standards.
* upsampling cpp is useless and it simply forwards to resize cpp. If we remove resize cpp, it is automatically removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577929141
btw, we all love python. Perhaps the final solution is we use tvm to compile these python descriptions XD so we don’t need to to have the debate. Thanks everyone for great inputs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] icemelon9 commented on issue #4769: [TOPI] Remove
cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
icemelon9 commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577910874
I agree with @masahi. During the work of #4644, I found a few inconsistency between cpp and python topi implementation. Even for some simple schedule like injective, python version is better than cpp version, and has already overwritten the cpp implementation. I feel that keeping both cpp and python implementation for topi is a bit confusing, even that one op may only exist in cpp or python. Given that most of people now contribute to topi python, it makes sense that we only maintain the python version.
In response to @tqchen, op compute could be stable, but the schedule might not be. Even op compute doesn't change over the time, we can still improve its schedule for better performance. It's easier to do so in python than in cpp.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577710438
I do not mean that we have do it in this PR. Indeed having a C++ impl would be harder than the python version.
On the other hand, it brings certain benefit to help the compiler to be more language agnostic, so this is a direction that we should strive to push for.
Given that the current cxx solution is incomplete, we could remove it, but if in the future we could have an effort to bring a stable cxx only solution, we might want to favor that one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577882904
Choosing cpp vs python per operator (rather than one lang for all ops) is a good compromise, but it creates other problems (need to discuss which impl to keep for each op, not obvious to newcomers which lang a particular op is implemented in, etc). But we can talk about that later.
So, going back to this PR, I propose to remove cpp resize and upsampling because,
* resize op is highly unstable. Recently people added new functionalities to resize python to support ONNX or Tensorflow use cases. ONNX, the one I'm familiar with, has a dozen of options that we have no support for. It is likely that we need to update our resize definition to cover more new or esoteric use cases from higher level frameworks / standards.
* upsampling cpp is useless and it simply forwards to resize cpp. If we remove resize cpp, it is automatically removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577930418
so tvm will finally compile itself :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577870143
I do agree that the cpp solution is desirable and has many benefits. For operators whose api and implementation would not be expected to change (pooling, elemwise op etc) it makes sense to have them in cpp only (and that is what we have currently). I also liked the recent effort which brought most of the relay build pipeline to cpp. A few days ago I sent a [PR](https://github.com/apache/incubator-tvm/pull/4751) which exposed some relay cpp functionality to python. Now that things are in cpp, this "exposing" can be done from any language (at least in principle).
C++ on topi is a different story :) I believe the cpp only solution is simply not realistic because of "human problem". We always say cpp is something "to strive for" or "to port over in the future", but it's never been a hard requirement. Also it is not clear what makes API "stable enough" to deserve a cpp impl. As soon as people find a reason not to do cpp, the cpp impl never happens. Most people who add features to topi just want to get their immediate jobs done, so "implement it in cpp" is usually not high on their list.
This ambiguity has created situations that our cpp resize op is in. In addition to the "outdated cpp" problem, there are many topi operators that only exist in python, including
* all sparse ops
* most of the vision specific ops (rcnn, ssd, deformable conv) except for the reorg op (for yolo)
* int8 and other lower bit ops
* framework specific ops (depth to space, fifo buffer etc)
and I'm sure there are others.
In summary,
* I agree that the cpp only solution is desirable and something we should strive for, but in practice not realistic
* Already too many inconsistencies on python vs cpp in topi. Most likely porting effort would not happen.
* The python only solution is the only viable option and basically it is already our status quo. We can keep the existing cpp impls that are useful, but we should make our python impl as the official one. This is also in line with the recent "Unified IR" initiative by @tqchen which, if I remember correctly, brings TVM to be more "Python first".
I usually don't have a strong opinion on things, but I believe this topi python vs cpp duplication and inconsistency situation is the most ugly corner in our code base and so wanted to say something about it. I'd love to hear other opinions :) @jwfromm @yzhliu @icemelon9
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577874461
Hmm, i hear you. I think the nature of “need to adding new features” marks the op unstable, thus merit a python version as the primary one.
On the other hand, for most of the numpy style ops, whose interface are stable, it makes sense to encourage c++ version and use that as default(while removing the python version)
In both cases, for each op there is one primary language for the op, depending on the status.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #4769: [TOPI]
Remove cpp upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577882904
Choosing cpp vs python per operator (rather than one lang for all ops) is a good compromise, but it creates other problems (need to discuss which impl to keep for each op, not obvious to newcomers which lang a particular op is implemented in, etc). But we can talk about that later.
So, going back to this PR, I propose to remove cpp resize and upsampling because,
* resize op is highly unstable. Recently people added new functionalities to resize python to support ONNX or Tensorflow use cases. ONNX, the one I'm familiar with, has a dozen of options that we have no support for. It is likely that we need to update our resize definition to cover more new or esoteric use cases from higher level frameworks / standards. resize cpp haven't got any update and it can only do nearest/bilinear resize.
* upsampling cpp is useless and it simply forwards to resize cpp. If we remove resize cpp, it is automatically removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577927317
will wait for others to give opinions, will merge tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577646627
In the case of upsampling and resize though should we consider them as stable ops and keep only cxx impl instead ?
I agree it is bad to keep both versions, would be great if we can decide on a version to keep
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577874461
Hmm, i hear you. I think the nature of “need to adding new features” marks the op unstable, thus merit a python version as the primary one.
On the other hand, for most of the numpy style ios, whose interface is stable, it makes sense to encourage c++ version and use that as default(while removing the python version)
In both cases, for each op there is one primary language for the op, depending on the status.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769#issuecomment-577921569
We can have another discussion thread about the scope of ops that goes to cxx.
My current take is that the decision will likely be made as per group basis, eg all numpy ops could goes to c++ as numpy interface is pretty stable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] vinx13 merged pull request #4769: [TOPI] Remove cpp
upsampling and resize op
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #4769: [TOPI] Remove cpp upsampling and resize op
URL: https://github.com/apache/incubator-tvm/pull/4769
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services