-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[GraphBolt] Fix hetero sampling bug with single fanout. #7719
Conversation
To trigger regression tests:
|
for i in range(11): | ||
nodes = {"u": torch.randint(10, (100,), device=F.ctx())} | ||
sampler(nodes, fanouts=torch.tensor([-1])) | ||
# Should reach here without crashing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing. Better not rely on crash for unit test. Try assert something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I assert in this test? I took the crashing code and made it into a test. I thought adding this test is better than not adding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the crash is great. For test_sample_neighbors_hetero_single_fanout, you can assert the sampled number of node is expected or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit comment].
Description
The code was simply wrong. I don't know why the tests didn't catch it.
Fixes https://discuss.dgl.ai/t/neighbour-sampling-on-cpu-crashes-for-graphbolt-heterographs/4521.
Checklist
Please feel free to remove inapplicable items for your PR.
Changes