You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "haoyang9804 (via GitHub)" <gi...@apache.org> on 2023/09/11 02:58:18 UTC
[GitHub] [tvm] haoyang9804 opened a new pull request, #15723: fix _convert_simple_rnn
haoyang9804 opened a new pull request, #15723:
URL: https://github.com/apache/tvm/pull/15723
Fiz this [issue](https://github.com/apache/tvm/issues/14868)
I found that `_convert_simple_rnn` has some logical errors. I'm not very sure if I fix it correctly. All in all, after this fix, running the following bug-triggered script
```Python
import tvm
import tvm.relay as relay
from tensorflow import keras
from tensorflow.keras import layers, models
input_shape = (2, 2, 2)
x = layers.Input(shape=input_shape[1:], dtype='float32')
layer = keras.layers.SimpleRNN(units=2)
layer.set_weights(layer.get_weights())
y = layer(x)
model = models.Model(x, y)
model.summary()
mod, params = relay.frontend.from_keras(model, {'input_1': input_shape})
mod = relay.transform.InferType()(mod)
print(mod)
with tvm.transform.PassContext(opt_level=3):
model = relay.build_module.create_executor("vm", mod, tvm.cpu(0), 'llvm', params).evaluate()
```
The compilation result is
```Python
Model: "model"
_________________________________________________________________
Layer (type) Output Shape Param #
=================================================================
input_1 (InputLayer) [(None, 2, 2)] 0
simple_rnn (SimpleRNN) (None, 2) 10
=================================================================
Total params: 10 (40.00 Byte)
Trainable params: 10 (40.00 Byte)
Non-trainable params: 0 (0.00 Byte)
_________________________________________________________________
def @main(%input_1: Tensor[(2, 2, 2), float32] /* ty=Tensor[(2, 2, 2), float32] */, %v_param_2: Tensor[(2, 4), float32] /* ty=Tensor[(2, 4), float32] */, %v_param_4: Tensor[(2), float32] /* ty=Tensor[(2), float32] */, %v_param_1: Tensor[(1, 2), float32] /* ty=Tensor[(1, 2), float32] */, %v_param_3: Tensor[(2, 2), float32] /* ty=Tensor[(2, 2), float32] */) -> Tensor[(2, 2), float32] {
%0 = nn.batch_flatten(%input_1) /* ty=Tensor[(2, 4), float32] */;
%1 = nn.dense(%0, %v_param_2, units=2) /* ty=Tensor[(2, 2), float32] */;
%2 = nn.bias_add(%1, %v_param_4) /* ty=Tensor[(2, 2), float32] */;
%3 = split(%2, indices_or_sections=[1], axis=1) /* ty=(Tensor[(2, 1), float32], Tensor[(2, 1), float32]) */;
%4 = nn.batch_flatten(%v_param_1) /* ty=Tensor[(1, 2), float32] */;
%5 = %3.0 /* ty=Tensor[(2, 1), float32] */;
%6 = nn.dense(%4, %v_param_3, units=2) /* ty=Tensor[(1, 2), float32] */;
%7 = add(%5, %6) /* ty=Tensor[(2, 2), float32] */;
%8 = %3.0 /* ty=Tensor[(2, 1), float32] */;
%9 = nn.dense(%7, %v_param_3, units=2) /* ty=Tensor[(2, 2), float32] */;
add(%8, %9) /* ty=Tensor[(2, 2), float32] */
}
One or more operators have not been tuned. Please tune your model for better performance. Use DEBUG logging level to see more details.
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1716185348
Seems that everything is good except for gpu/docs ci tests. May I ask how to fix it? I never met this issue before @vvchernov.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1713181791
Hello @haoyang9804! I've looked through your code, but I'm not so familiar with it. It is good that you add test and it works succussfully, but I should warn that numpy does not work with **bfloat16**, but TVM and other frameworks do.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1715647907
cc @echuraev
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on pull request #15723: fix _convert_simple_rnn
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1716339670
@haoyang9804, I have restarted CI. In case of further errors, please try to rebase your branch to the latest mainline.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on code in PR #15723:
URL: https://github.com/apache/tvm/pull/15723#discussion_r1321024911
##########
python/tvm/relay/frontend/keras.py:
##########
@@ -1053,22 +1053,30 @@ def _convert_simple_rnn(
in_data = inexpr[0]
prev_op = inexpr[1]
weightList = keras_layer.get_weights()
- kernel_weight = etab.new_const(weightList[0].transpose([1, 0]))
+ weightList0 = weightList[0].transpose([1, 0])
+ assert len(in_data.type_annotation.shape) == 3
+ for i in range(in_data.type_annotation.shape[1].value - 1):
+ weightList0 = np.hstack((weightList0, weightList[0].transpose([1, 0])))
Review Comment:
Could you check your code for bfloat16 weights? numpy.hstack has dtype arg and I guess it possibly checks it if so numpy fails when dtype is bfloat16
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on a diff in pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on code in PR #15723:
URL: https://github.com/apache/tvm/pull/15723#discussion_r1321289332
##########
python/tvm/relay/frontend/keras.py:
##########
@@ -1053,22 +1053,30 @@ def _convert_simple_rnn(
in_data = inexpr[0]
prev_op = inexpr[1]
weightList = keras_layer.get_weights()
- kernel_weight = etab.new_const(weightList[0].transpose([1, 0]))
+ weightList0 = weightList[0].transpose([1, 0])
+ assert len(in_data.type_annotation.shape) == 3
+ for i in range(in_data.type_annotation.shape[1].value - 1):
+ weightList0 = np.hstack((weightList0, weightList[0].transpose([1, 0])))
Review Comment:
It's true that numpy.hstack does not support bfloat16. But weightList[0] and weightList0 can never be bfloat16 to my best understanding. These two vars are from weightList = keras_layer.get_weights(), and they are NumPy arrays. If numpy does not support bfloat16, I think the dtype of weightList[0] should never be bfloat16. So this worry seems unnecessary here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on pull request #15723: fix _convert_simple_rnn
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1716338282
@tvm-bot rerun
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on code in PR #15723:
URL: https://github.com/apache/tvm/pull/15723#discussion_r1321013935
##########
python/tvm/relay/frontend/keras.py:
##########
@@ -1053,22 +1053,30 @@ def _convert_simple_rnn(
in_data = inexpr[0]
prev_op = inexpr[1]
weightList = keras_layer.get_weights()
- kernel_weight = etab.new_const(weightList[0].transpose([1, 0]))
+ weightList0 = weightList[0].transpose([1, 0])
+ assert len(in_data.type_annotation.shape) == 3
+ for i in range(in_data.type_annotation.shape[1].value - 1):
+ weightList0 = np.hstack((weightList0, weightList[0].transpose([1, 0])))
Review Comment:
I'm aware of this line
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1716250601
Hello @echuraev! Looks like it isproblem from jenkins:
```[2023-09-12T17:54:08.315Z] + ./ci/scripts/jenkins/s3.py --action upload --bucket tvm-jenkins-artifacts-prod --prefix tvm/PR-15723/3/pytest-results/docs_GPU --items build/pytest-results
[2023-09-12T17:54:08.315Z] [ci/scripts/jenkins/s3.py:99 INFO] Namespace(action='upload', bucket='tvm-jenkins-artifacts-prod', items=['build/pytest-results'], prefix='tvm/PR-15723/3/pytest-results/docs_GPU')
[2023-09-12T17:54:08.315Z] [ci/scripts/jenkins/s3.py:109 INFO] Using s3 path: s3://tvm-jenkins-artifacts-prod/tvm/PR-15723/3/pytest-results/docs_GPU
[2023-09-12T17:54:08.315Z] Traceback (most recent call last):
[2023-09-12T17:54:08.315Z] File "./ci/scripts/jenkins/s3.py", line 150, in <module>
[2023-09-12T17:54:08.315Z] raise RuntimeError(f"Cannot upload empty folder with name: {item}")
[2023-09-12T17:54:08.315Z] RuntimeError: Cannot upload empty folder with name: build/pytest-results```
Do you know who can help us?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on a diff in pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on code in PR #15723:
URL: https://github.com/apache/tvm/pull/15723#discussion_r1321019116
##########
python/tvm/relay/frontend/keras.py:
##########
@@ -1053,22 +1053,30 @@ def _convert_simple_rnn(
in_data = inexpr[0]
prev_op = inexpr[1]
weightList = keras_layer.get_weights()
- kernel_weight = etab.new_const(weightList[0].transpose([1, 0]))
+ weightList0 = weightList[0].transpose([1, 0])
+ assert len(in_data.type_annotation.shape) == 3
+ for i in range(in_data.type_annotation.shape[1].value - 1):
+ weightList0 = np.hstack((weightList0, weightList[0].transpose([1, 0])))
Review Comment:
Could you plz elaborate? I still cannot see any data type issue here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on a diff in pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on code in PR #15723:
URL: https://github.com/apache/tvm/pull/15723#discussion_r1321046482
##########
python/tvm/relay/frontend/keras.py:
##########
@@ -1053,22 +1053,30 @@ def _convert_simple_rnn(
in_data = inexpr[0]
prev_op = inexpr[1]
weightList = keras_layer.get_weights()
- kernel_weight = etab.new_const(weightList[0].transpose([1, 0]))
+ weightList0 = weightList[0].transpose([1, 0])
+ assert len(in_data.type_annotation.shape) == 3
+ for i in range(in_data.type_annotation.shape[1].value - 1):
+ weightList0 = np.hstack((weightList0, weightList[0].transpose([1, 0])))
Review Comment:
Oh I see. I will check it later. Thx
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1713196512
> Hello @haoyang9804! I've looked through your code, but I'm not so familiar with it. It is good that you add test and it works succussfully, but I should warn that numpy does not work with **bfloat16**, but TVM and other frameworks do.
Thanks for the reply. But I think my patch is not related to **bfloat16**
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev merged pull request #15723: fix _convert_simple_rnn
Posted by "echuraev (via GitHub)" <gi...@apache.org>.
echuraev merged PR #15723:
URL: https://github.com/apache/tvm/pull/15723
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] haoyang9804 commented on pull request #15723: fix _convert_simple_rnn
Posted by "haoyang9804 (via GitHub)" <gi...@apache.org>.
haoyang9804 commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1713084594
cc @vvchernov @echuraev
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #15723: fix _convert_simple_rnn
Posted by "vvchernov (via GitHub)" <gi...@apache.org>.
vvchernov commented on PR #15723:
URL: https://github.com/apache/tvm/pull/15723#issuecomment-1716266284
I've rechecked PRs: #15714 and #15709 have the same issue, but the next PRs do not have. Possibly it is the best way to restart this 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org