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

fix to skip node not in graph. #5238

Merged
merged 1 commit into from
Apr 6, 2020
Merged

fix to skip node not in graph. #5238

merged 1 commit into from
Apr 6, 2020

Conversation

chinakook
Copy link
Contributor

fix to skip node not in graph because some network cannot be hybridized with some var unused.

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

fix to skip node not in graph because some network cannot be hybridized with some var unused.
@icemelon
Copy link
Member

icemelon commented Apr 5, 2020

Could you add some test case for it?

@chinakook
Copy link
Contributor Author

chinakook commented Apr 6, 2020

@icemelon9 what kind of tests, please? Failure case I've met with?

@tqchen tqchen merged commit 3e8c7be into apache:master Apr 6, 2020
@tqchen
Copy link
Member

tqchen commented Apr 6, 2020

I am going to merge this for now, given it is a change to the nnvm code. However, based on my current impression, I am not really sure if such relaxation should be done, given that node2index is supposed to only contain nodes in the graph, @chinakook would be great if you can comment a bit.

@chinakook
Copy link
Contributor Author

@tqchen As I'v just tested, It's a op named _FusedOp that cannot be CHECK success.

@tqchen
Copy link
Member

tqchen commented Apr 6, 2020

i see, perhaps you also want to confirm with the mxnet community as well to make sure it is the intended behavior cc @szha

@chinakook
Copy link
Contributor Author

I'm sure some network will be fail to hybridize after this apache/mxnet#15167 PR. So we can add e.node->op()->name.compare("_FusedOp")==0 at first. However, this op name is specified to mxnet but not tvm.

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
fix to skip node not in graph because some network cannot be hybridized with some var unused.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
fix to skip node not in graph because some network cannot be hybridized with some var unused.
dpankratz pushed a commit to dpankratz/incubator-tvm that referenced this pull request Apr 24, 2020
fix to skip node not in graph because some network cannot be hybridized with some var unused.
@ptrendx
Copy link
Member

ptrendx commented Oct 12, 2020

This is not right and should be reverted IMO - the problem in the original issue (apache/mxnet#18624) was a genuine bug in the fusion graph pass that generated a cycle in the graph. Omitting such node from indexed graph would result in incorrect result being produced.

@tqchen
Copy link
Member

tqchen commented Oct 12, 2020

@ptrendx feel free to send a PR to revert the change then

ptrendx added a commit to ptrendx/tvm that referenced this pull request Oct 13, 2020
@ptrendx ptrendx mentioned this pull request Oct 13, 2020
@ptrendx
Copy link
Member

ptrendx commented Oct 13, 2020

@tqchen PR #6680 done.

tqchen pushed a commit that referenced this pull request Oct 14, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Oct 29, 2020
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants