Skip to content

Conversation

@jikechao
Copy link
Member

@jikechao jikechao commented Jul 17, 2023

This PR fixes two types of bugs:
Bug 1. fix a typo about the wrong API name in the test case.
Bug 2-5. add an assertion to reject the invalid model, which can be constructed in the old version of Keras. The attribute units in LSTM, RNN, GRU, and Dense must have a positive value. Such invalid values can be used to construct a DL model successfully and can be converted to RelayIR successfully. In this pr, we reject these invalid models containing such layers.

Steps to reproduce to bug-2 about LSTM: (Keras <= 2.6)

import tvm
import tvm.relay as relay
import numpy as np
from tensorflow import keras
from tensorflow.keras import layers, models

input_shape = (1, 2, 4)
input_data = np.random.random(size=input_shape)
dtype = 'float32'

x = layers.Input(shape=input_shape[1:], dtype=dtype)

layer = keras.layers.LSTM(units=0)        # invalid input but can not reject in keras <= 2.4
layer.set_weights(layer.get_weights())

y = layer(x)
model = models.Model(x, y)
res_keras = model.predict(input_data)

print(model.summary())
res_keras2 = model(input_data)

shape_dict = {'input_1': input_shape}
mod, params = relay.frontend.from_keras(model, shape_dict)
print(mod)

In the latest version of Keras, the invalid value for units will be rejected. Thus, the test case can be triggered using the Keras <=2.4.
Thus, we cannot new test cases into the unit tests.

cc @echuraev @leandron

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jul 17, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@jikechao jikechao changed the title [Bugfix][Relay][Frontend][Keras] Add a assertion to reject a invalid layer which was ignored in old version [Bugfix][Relay][Frontend][Keras] Add a assertion to reject a invalid layer which was ignored in old version Keras Jul 17, 2023
@github-actions github-actions bot requested review from echuraev and leandron July 17, 2023 05:57
@jikechao jikechao changed the title [Bugfix][Relay][Frontend][Keras] Add a assertion to reject a invalid layer which was ignored in old version Keras [Bugfix][Relay][Frontend][Keras] Add a assertion to reject a invalid value for attribute units in RNN layers Jul 17, 2023
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM. I have some doubts. How did these models with incorrect units number work in old versions of Keras? I'm afraid that we can break a backward compatibility by this patch. Probably we could process this units number in the way how it was implemented in Keras? Or we can check Keras version and generate assert for newer versions of Keras. What do you think?

@jikechao
Copy link
Member Author

@echuraev Thanks for your comments!

If unit =0, the output shape will contain a zero dimension. Such models will be meaningless. It looks like a bug in the old version of Keras, and this bug was fixed in the later version. Thus, I would like to reject these invalid models.
image

In the new version of Keras, the called statement layer = keras.layers.LSTM(units=0) will crash immediately by checking in Keras. Thus, the newly added assertion statement only works for that old version of Keras .

@jikechao
Copy link
Member Author

BTW, in addition to LSTM, RNN, and GRU, the Dense has the same bug. Thus we added an extra patch to fix _convert_dense simultaneously!

@jikechao jikechao requested a review from echuraev July 17, 2023 18:08
@jikechao
Copy link
Member Author

@tvm-bot rerun

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarification. LGTM.

@jikechao
Copy link
Member Author

@Hzfengsy @echuraev @leandron
CI passed. Could you help me merge this PR?

@Hzfengsy Hzfengsy merged commit 8e33401 into apache:main Jul 20, 2023
@Hzfengsy
Copy link
Member

Thanks for the fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants