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.

Sign up for GitHub

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

Jump to bottom

Tutorial: update Building a Graph Convolutional Network tutorial #4060

Merged
merged 9 commits into from
Oct 11, 2019
Merged

Tutorial: update Building a Graph Convolutional Network tutorial #4060

merged 9 commits into from
Oct 11, 2019

Conversation

cylinbao
Copy link
Contributor

@cylinbao cylinbao commented Oct 4, 2019

Few updates on the Building a Graph Convolutional Network tutorial

  • Add bias in the GCN layer
  • Load pretrained model exported from DGL and verify model accuracy
  • use time_evaluator to measure the runtime

Please review
@ZihengJiang, @tmoreau89

@cylinbao
update build_gcn.py tutorial
e428bd4 
updates
* support bias in GCN layer
* download pretrained gcn model
* verify model accuracy
* use time_evaluator to measure runtime
@cylinbao
fix adding bias in gcn layer
e5f9bad
@cylinbao
remove printing output
6a78998
@cylinbao
fix small bug
c39bb09
@tmoreau89
Copy link
Contributor

Adding @yuluny2 to also review the PR.

tmoreau89
tmoreau89 reviewed Oct 7, 2019
View reviewed changes
tutorials/frontend/build_gcn.py Outdated

# Evaluate the runtime
print("Evaluate inference time cost...")
timer = m.module.time_evaluator("run", ctx, number=1, repeat=10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Increase the number

tmoreau89
tmoreau89 requested changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@tmoreau89 tmoreau89 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 @cylinbao for these changes. Can we run inside of this tutorial the DGL MxNet/Pytorch baseline and compare the results as well as the runtimes?

Also can we control GPU target with a flag to show both modes of operation?

@yy665
Copy link
Contributor

yy665 commented Oct 7, 2019

lgtm

@cylinbao
Copy link
Contributor Author

cylinbao commented Oct 7, 2019

Thank @tmoreau89 for the comments.
There is no stable GPU scheduling yet, so we can't show the GPU version.

@cylinbao
Copy link
Contributor Author

cylinbao commented Oct 7, 2019

@tmoreau89
In my personal testing, TVM runs faster than MxNet but has similar speed with PyTorch.
Also, the inference time is very short (under 30ms) for each iteration.
I'm not sure whether it's fair to show the runtime comparison.

@ZihengJiang
Copy link
Contributor

@cylinbao better to change the previous performance claim

ZihengJiang
ZihengJiang approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@ZihengJiang ZihengJiang left a comment

Choose a reason for hiding this comment

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

LGTM

@tmoreau89
Copy link
Contributor

@cylinbao thanks. If you can just submit a correctness check that will ensure that our computation is correct. Let's leave GPU support and performance comparison to a future PR.

@cylinbao
Copy link
Contributor Author

cylinbao commented Oct 8, 2019

@tmoreau89
Add the checking with DGL-PyTorch.

@ZihengJiang @yuluny2
You may want to review again. Thanks!

@tmoreau89
Copy link
Contributor

Thanks looks good, please address the comments above.

@cylinbao
Copy link
Contributor Author

cylinbao commented Oct 8, 2019

I think all the comments have been addressed!

@tmoreau89
Copy link
Contributor

Thanks, indeed they've been addressed (I resolved them). Will wait on checks to pass before merging it in!

@tmoreau89
Copy link
Contributor

tmoreau89 commented Oct 9, 2019 •
edited
Loading

Looks like the CIs failed, can you investigate @cylinbao?

@cylinbao
Copy link
Contributor Author

cylinbao commented Oct 9, 2019 •
edited
Loading

It seems like the DGL version is too old in the CLs.
Is there a way we can update the DGL version to 0.4?

@cylinbao
Copy link
Contributor Author

Add one commit to handle the DGL version issue in the code.

@tmoreau89
Copy link
Contributor

Thank you for the update @cylinbao !

@tmoreau89 tmoreau89 merged commit ef66653 into apache:master Oct 11, 2019
anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
@cylinbao
Tutorial: update Building a Graph Convolutional Network tutorial (apa…
ba30754 
…che#4060)

* update build_gcn.py tutorial

updates
* support bias in GCN layer
* download pretrained gcn model
* verify model accuracy
* use time_evaluator to measure runtime

* fix adding bias in gcn layer

* remove printing output

* fix small bug

* add DGL-PyTorch comparison into the build_gcn tutorial

* add accuracy testing

* adjust import order

* handle different dgl versions

* update number for dgl version checking
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 18, 2019
@cylinbao @wweic
Tutorial: update Building a Graph Convolutional Network tutorial (apa…
e4fa297 
…che#4060)

* update build_gcn.py tutorial

updates
* support bias in GCN layer
* download pretrained gcn model
* verify model accuracy
* use time_evaluator to measure runtime

* fix adding bias in gcn layer

* remove printing output

* fix small bug

* add DGL-PyTorch comparison into the build_gcn tutorial

* add accuracy testing

* adjust import order

* handle different dgl versions

* update number for dgl version checking
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZihengJiang ZihengJiang ZihengJiang approved these changes

@cchung100m cchung100m cchung100m approved these changes

@tmoreau89 tmoreau89 tmoreau89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cylinbao @tmoreau89 @yy665 @ZihengJiang @cchung100m