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/04/16 12:35:43 UTC
[GitHub] [incubator-tvm] mbaret opened a new pull request #5345: [RELAY]
Move frontend utils
mbaret opened a new pull request #5345: [RELAY] Move frontend utils
URL: https://github.com/apache/incubator-tvm/pull/5345
The util file currently under frontend is used from outside of frontend (in qnn/op/legalizations). This suggests that the file should be pushed up to a higher level.
The benefit from this change is that importing qnn no longer also imports all the frontends. I've run into this trying to import qnn from within op.contrib.
----------------------------------------------------------------
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] mbaret commented on issue #5345: [RELAY] Move
frontend utils
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5345: [RELAY] Move frontend utils
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-614706054
cc @anijain2305 @zhiics
----------------------------------------------------------------
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] mbaret commented on a change in pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#discussion_r417275917
##########
File path: python/tvm/relay/qnn/op/legalizations.py
##########
@@ -20,8 +20,8 @@
import tvm
from tvm import relay
+import numpy as np
Review comment:
If you take a look here:
https://github.com/apache/incubator-tvm/blob/063ba63a4448432a1e17ec8b793152cd8bba7c2c/docker/install/ubuntu_install_python_package.sh#L24
CI runs with pylint 1.9.4 and I imagine that's where the difference is coming from.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-618893040
I couldn't find an appropriate folder to move this to, so I've instead inlined this function as it's only used in two places.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] Somisary removed a comment on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
Somisary removed a comment on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616730802
Where would be appropriate? An alternative would be that given this is used only in two places we could inline it into those files.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics edited a comment on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616769231
Yeah, I meant we probably don't want to put under relay root folder, somewhere else (even some duplication in each file) is okay to me.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] mbaret commented on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616650764
Does anyone have a chance to take a look at this?
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616725734
I am actually not quite encourage us to introduce this to the relay namespace as this file only really has one function and it is rarely used
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#discussion_r414572622
##########
File path: python/tvm/relay/frontend/tflite.py
##########
@@ -2219,6 +2218,20 @@ def get_expr(self, input_tensor_idx):
def has_expr(self, input_tensor_idx):
return self.exp_tab.has_expr(get_tensor_name(self.subgraph, input_tensor_idx))
+
+def get_scalar_from_constant(expr):
+ """ Returns scalar value from Relay constant scalar. """
+ assert isinstance(expr, _expr.Constant) and not expr.data.shape, \
+ "Expr is not a constant scalar."
+ value = expr.data.asnumpy()
+ if value.dtype == np.dtype(np.int32):
+ return int(value)
+ if value.dtype == np.dtype(np.float32):
+ return float(value)
+ assert False, "Constant expr must be float32/int32"
+ return None # To suppress pylint
Review comment:
I am okay with this type of minimal code duplication.
Can we just do this for both cases?
```python
assert value.dtype == np.dtype(np.int32) or value.dtype == np.dtype(np.float32), "value must be float32/int32"
return np. asscalar(value)
```
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] roastduck commented on a change in pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
roastduck commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#discussion_r417265081
##########
File path: python/tvm/relay/qnn/op/legalizations.py
##########
@@ -20,8 +20,8 @@
import tvm
from tvm import relay
+import numpy as np
Review comment:
I run `./tests/scripts/task_lint.sh` directly from the repository root.
My `pylint` version:
```
pylint 2.4.4
astroid 2.3.3
Python 3.7.0 (default, Mar 12 2019, 20:10:09)
[GCC 7.3.0]
```
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] roastduck commented on a change in pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
roastduck commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#discussion_r416555638
##########
File path: python/tvm/relay/qnn/op/legalizations.py
##########
@@ -20,8 +20,8 @@
import tvm
from tvm import relay
+import numpy as np
Review comment:
A minor problem: Lint reports `python/tvm/relay/qnn/op/legalizations.py:23: [C0411(wrong-import-order), ] third party import "import numpy as np" should be placed before "import tvm"` on my platform. I have no idea why this PR can pass CI.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] tqchen commented on pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-618773499
please suggest a resolution(move to a new folder) and act on it :)
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616769231
Yeah, I meant we probably don't want to put under relay root folder, somewhere else is okay to me.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] Somisary commented on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
Somisary commented on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616730802
Where would be appropriate? An alternative would be that given this is used only in two places we could inline it into those files.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#discussion_r414572622
##########
File path: python/tvm/relay/frontend/tflite.py
##########
@@ -2219,6 +2218,20 @@ def get_expr(self, input_tensor_idx):
def has_expr(self, input_tensor_idx):
return self.exp_tab.has_expr(get_tensor_name(self.subgraph, input_tensor_idx))
+
+def get_scalar_from_constant(expr):
+ """ Returns scalar value from Relay constant scalar. """
+ assert isinstance(expr, _expr.Constant) and not expr.data.shape, \
+ "Expr is not a constant scalar."
+ value = expr.data.asnumpy()
+ if value.dtype == np.dtype(np.int32):
+ return int(value)
+ if value.dtype == np.dtype(np.float32):
+ return float(value)
+ assert False, "Constant expr must be float32/int32"
+ return None # To suppress pylint
Review comment:
I am okay with this type of minimal code duplication.
Can we just do for both case?
```python
assert value.dtype == np.dtype(np.int32) or value.dtype == np.dtype(np.float32), "value must be float32/int32"
return np. asscalar(value)
```
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] zhiics edited a comment on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616725734
I am actually not quite encouraging us to introduce this to the relay namespace as this file only really has one function and it is rarely used
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] anijain2305 commented on issue #5345: [RELAY] Move frontend utils
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on issue #5345:
URL: https://github.com/apache/incubator-tvm/pull/5345#issuecomment-616753221
Maybe we want to move it some other already existing file. But, I think we need this in Relay namespace. This is used in TFLite parser and QNN Op legalization.
~~~
python/tvm/relay/frontend/tflite.py:from .util import get_scalar_from_constant
python/tvm/relay/frontend/tflite.py: lhs_scale_value = get_scalar_from_constant(lhs_scale)
python/tvm/relay/frontend/tflite.py: rhs_scale_value = get_scalar_from_constant(rhs_scale)
python/tvm/relay/frontend/tflite.py: lhs_zero_point_value = get_scalar_from_constant(lhs_zero_point)
python/tvm/relay/frontend/tflite.py: rhs_zero_point_value = get_scalar_from_constant(rhs_zero_point)
python/tvm/relay/frontend/tflite.py: data_scale_val = get_scalar_from_constant(data_scale)
python/tvm/relay/frontend/tflite.py: weight_scale_val = get_scalar_from_constant(weight_scale)
python/tvm/relay/frontend/tflite.py: pad_value = get_scalar_from_constant(input_tensor.qnn_params['zero_point'])
python/tvm/relay/frontend/tflite.py: data_scale_val = get_scalar_from_constant(data_scale)
python/tvm/relay/frontend/tflite.py: weight_scale_val = get_scalar_from_constant(weight_scale)
python/tvm/relay/frontend/util.py:def get_scalar_from_constant(expr):
python/tvm/relay/qnn/op/legalizations.py:from ...frontend.util import get_scalar_from_constant
python/tvm/relay/qnn/op/legalizations.py: zero_point_val = get_scalar_from_constant(zero_point)
python/tvm/relay/qnn/op/legalizations.py: zero_point_val = get_scalar_from_constant(zero_point)
~~~
----------------------------------------------------------------
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