Skip to content

[FRONTEND][TENSORFLOW] Add extra_param to from_tensorflow#8556

Closed
jcf94 wants to merge 6 commits intoapache:mainfrom
jcf94:tf_extra_param
Closed

[FRONTEND][TENSORFLOW] Add extra_param to from_tensorflow#8556
jcf94 wants to merge 6 commits intoapache:mainfrom
jcf94:tf_extra_param

Conversation

@jcf94
Copy link
Contributor

@jcf94 jcf94 commented Jul 26, 2021

In some graph, for example:

    g = tf.Graph()
    a_shape = [None, 30]
    b_shape = [30, 40]
    batch_size = 25
    static_a_shape = [batch_size, 30]
    dtype = "float32"
    with g.as_default():
        A = tf.placeholder(shape=a_shape, dtype=dtype, name="A")
        B = tf.placeholder(shape=b_shape, dtype=dtype, name="B")
        C = tf.matmul(A, B)
        batch = tf.placeholder(shape=(), dtype="int32", name="batch_size")
        batch = tf.expand_dims(batch, axis=0)
        shape = tf.concat([batch, tf.constant([5]), tf.constant([8])], axis=0)
        D = tf.reshape(C, shape)

We have input placeholder batch_size whose value would be used to determine the shape of another op. Even though the shape of tensor_a can be specified through from_tensorflow(shape=(...)) and in this case the batch_size value must be a fixed number, we cannot process this as a static graph.

This PR adds an extra parameter to from_tensorflow, that we can manually fix a tensor to be a constant value, then the graph can be processed as a static graph.

@u99127
Copy link

u99127 commented Jul 27, 2021

@leandron - is there a need to check this usage via tvmc as well if this changes ?

Re-triggle CI
@junrushao
Copy link
Member

CC @yongwww @comaniac

@comaniac
Copy link
Contributor

comaniac commented Aug 2, 2021

AFAIK, other frontends also have the similar issue. For example, ONNX graph also put some shapes to the model parameters, but their constant values are maintained in the model artifacts when the model is frozen, so it may just need extra_param_names. As a result, instead of fixing this issue one-by-one, I'm wondering if we could have a unified solution. Specifically, can we do the following to achieve the same goal?

  1. Get a dynamic graph via a frontend.
  2. Bind parameters (i.e., batch_size) by name.
  3. Run DynamicToStatic to get a static graph.

It seems to me that the above flow should result in the same outcome without this PR (assuming DynamicToStatic is doing a good job). If that works, it could be the best practice for not only TF but all frameworks IMHO.

@jcf94
Copy link
Contributor Author

jcf94 commented Aug 6, 2021

Thanks to all!
Have discussed with @comaniac before, and seems we need a better solution for this. I'll close this for now, and try to solve with another approach.

@jcf94 jcf94 closed this Aug 6, 2021
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