Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Relay][TF] Keep node name in span #6885

Merged
merged 1 commit into from
Nov 9, 2020
Merged

[Relay][TF] Keep node name in span #6885

merged 1 commit into from
Nov 9, 2020

Conversation

lixiaoquan
Copy link
Contributor

With this PR, it is easy to trace a relay CallNode to its source in an imported model. It looks like:

%57 = nn.relu(%56) /* resnet_model/Relu_9 */;
%58 = transpose(%resnet_model/conv2d_12/kernel, axes=[3, 2, 0, 1]);
%59 = nn.conv2d(%57, %58, padding=[0, 0, 0, 0], channels=128, kernel_size=[1, 1]) /* resnet_model/conv2d_12/Conv2D */;
%60 = nn.batch_norm(%59, %resnet_model/batch_normalization_10/gamma, %resnet_model/batch_normalization_10/beta, %resnet_model/batch_normalization_10/moving_mean, %resnet_model/batch_normalization_10/moving_variance, epsilon=1.001e-05f) /* resnet_model/batch_normalization_10/FusedBatchNorm */;

I'm not sure whether it is the correct to use span.SourceName to store node name, but line, column doesn't make sense for an imported model.

@jroesch @junrushao1994 @zhiics Could you please help to review? Thanks

@FrozenGene
Copy link
Member

Ah...I think it is a nice feature and should enable it in all frontend, this is good for profiling (like we use debug graph runtime to see every layer's output). But I remember @tqchen said we will have another WIP feature to do this?

@tqchen
Copy link
Member

tqchen commented Nov 9, 2020

I now think that span is the right way to do it, as long as we don't rely the information in passes.

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM. I think this is also what @jroesch was adding.

@tqchen
Copy link
Member

tqchen commented Nov 9, 2020

Perhaps we could also do similar things for other frontends

@tqchen tqchen merged commit eb1fa29 into apache:main Nov 9, 2020
@jroesch
Copy link
Member

jroesch commented Nov 11, 2020

@lixiaoquan I think we can do this, but I am not sure if overloading the source spans are the right way? perhaps we can sub-class or modify spans for handling whether they point into a source file or into an imported graph? I think it might be good to clarify this design.

@lixiaoquan
Copy link
Contributor Author

@jroesch I agree with you. It looks like a makeshift to use SourceName to store node name, I did this because it introduced very little change. I think we can add a hint field. I added a thread https://discuss.tvm.apache.org/t/expand-span-for-imported-module/8435

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants