-
Notifications
You must be signed in to change notification settings - Fork 297
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
[BUG] Fix Issue in cuGraph-PyG Tests Blocking CI #3607
[BUG] Fix Issue in cuGraph-PyG Tests Blocking CI #3607
Conversation
…-nv/cugraph into fix-loader-ci-failure
…rkaround a numba 0.57 incompatibility.
…nd a breaking cudf change related to its numba dependency.
@@ -79,7 +78,7 @@ def karate_gnn(): | |||
el = karate.get_edgelist().reset_index(drop=True) | |||
el.src = el.src.astype("int64") | |||
el.dst = el.dst.astype("int64") | |||
all_vertices = np.array_split(cudf.concat([el.src, el.dst]).unique().values_host, 2) | |||
all_vertices = np.array_split(np.arange(34), 2) |
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.
This failure is probably caused by rapidsai/cudf#11656.
The ordering of cudf's unique
and drop_duplicates
has changed. Previously, the order was sorted. Now, the order is stable (preserves input order while removing duplicates). This may affect other parts of cuGraph, too, so I will take a look.
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.
Right, I was aware of that. Ironically, I have another PR which takes advantage of the new default behavior for drop_duplicates
.
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.
Yes, it looks like unique
is heavily used in cuGraph's Python code. We may need to review these for correctness if sorted order is expected. https://github.com/search?q=repo%3Arapidsai%2Fcugraph+unique%28+language%3APython&type=code&l=Python
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.
Awesome, thanks @alexbarghi-nv! Glad to hear the new behavior is helpful for your other PR! :D
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.
Approving doc updates from the https://github.com/bdice/cugraph/tree/numba-0.57 branch that had to be merged in.
/merge |
One of the fixtures for the cuGraph-PyG tests is not properly partitioning the edgelist, causing some tests to fail. This PR updates that fixture to correctly partition the edgelist.