[REVIEW] MNMG Louvain Python updates, Cython cleanup #1139

rlratzel · 2020-09-10T07:18:33Z

This adds a Python test and modules under cugraph/dask/community to support the MNMG Louvain C++ updates.
These changes are needed not only for exposing MG Louvain to users, but also to facilitate C++ testing.

This is still a WIP - it builds, but that's about it for now.
At a minimum, still need to:

Improve test
Figure out Dask calls needed and update dask/commuity/louvain.py accordingly
Call new 2D graph class from louvain_wrapper.pyx ([REVIEW] A new graph class to support 2D graph partitioning #1098)

Issue: #1052

This also includes some cleanup: renamed the graph_new* files to (hopefully) better reflect their contents. The new name is graph_primtypes* but this can easily be changed if there's a better name that should be used.

…s, removed unused graph_new.pxd (which was identical to graph.pxd, which then got renamed to graph_primtypes.pxd)

…y, etc.

GPUtester · 2020-09-10T07:19:02Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

…t the moment since it's not calling the Louvain C++ API which has not been merged yet.

afender

Can you elaborate on the motivation for renaming graph into graph_primtypes and why the latter is a better name?
Are we reserving cugraph.structure.graph for something else?

rlratzel · 2020-09-10T18:14:50Z

Can you elaborate on the motivation for renaming graph into graph_primtypes and why the latter is a better name?
Are we reserving cugraph.structure.graph for something else?

I initially wanted to use "graph", but I had some issues with the graph module that cython generated colliding with the current graph.py, but I admittedly didn't push too hard to get them merged after the import errors I initially saw after that attempt.

I just took a stab at a simple rename of graph_new at that point, as well as removing the redundant graph.pxd file. "primtypes" seemed like a decent fit based on the contents being the lower-level more primitive basic types used for graph algos.

I'm open to a better name, or even trying harder to combine the contents of graph.py with what the former graph_new cython modules were defining. I can even back this cleanup out if we want to revisit this later.

afender · 2020-09-10T18:26:27Z

Typically we add the _wrapper suffix for .pyx files to avoid conflicts with .py files. Would it help here? I noticed there was a pyx file that didn't have the suffix.
Ideally, we should keep the most concise name to be exposed and add prefixes/suffixes to those that don't get exposed.

rlratzel · 2020-09-10T20:39:01Z

Typically we add the _wrapper suffix for .pyx files to avoid conflicts with .py files. Would it help here? I noticed there was a pyx file that didn't have the suffix.

I tried using that pattern as well, but I had an issue combining the code in graph_new.pyx and graph_new_wrapper.pyx into graph_wrapper.pyx - cython crashed, and it seemed it didn't like having a cdef function that was used by one of the def functions in the same file (my theory). I didn't investigate much and I didn't spend time refactoring it either, and instead just decided to rename for clarity and remove the redundant graph.pxd file. Combining the defs and cdefs this way in a single file seems like it should be legal, but the only way I could get it to build was to keep them separate (graph_new was doing this as well). So we still have 3 files, but they have hopefully better names now: graph_primtypes.pxd, graph_primtypes.pyx (cdefs), and graph_primtypes_wrapper.pxd (defs).

I'd like to solve this better, but I'm afraid of taking too much time away from the MG work. I'm fine backing this out if it's too distracting now, or if we want to do it differently later.

…mglouvaintesting2

…t seems to be running and returning expected results now (based on comparison to non-dask SG test).

…rs to call it.

rlratzel · 2020-09-15T16:08:50Z

rerun tests

ChuckHastings

I think this is a good start. It would allow work to proceed in parallel while we explore things.

It would make sense for some of the things that are custom to Louvain in this PR to actually be more general purpose. Hopefully as we get more of the cython pieces in place it will become obvious which things should migrate to more generic functions.

cpp/include/utilities/cython.hpp

cpp/src/utilities/cython.cpp

codecov-commenter · 2020-09-15T17:44:21Z

Codecov Report

Merging #1139 into branch-0.16 will increase coverage by 0.24%.
The diff coverage is 96.42%.

@@               Coverage Diff               @@
##           branch-0.16    #1139      +/-   ##
===============================================
+ Coverage        72.36%   72.61%   +0.24%     
===============================================
  Files               56       58       +2     
  Lines             2135     2158      +23     
===============================================
+ Hits              1545     1567      +22     
- Misses             590      591       +1

Impacted Files	Coverage Δ
python/cugraph/dask/community/louvain.py	`94.73% <94.73%> (ø)`
python/cugraph/dask/__init__.py	`100.00% <100.00%> (ø)`
python/cugraph/dask/community/__init__.py	`100.00% <100.00%> (ø)`
python/cugraph/structure/graph.py	`80.22% <100.00%> (ø)`
python/cugraph/_version.py	`44.80% <0.00%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a24016d...82d2a3d. Read the comment docs.

…oped enum, removed dead code, removed unneeded std::move()

…mglouvaintesting2

afender

Good progress. There's still work to do on UX, testing, and documentation

afender · 2020-09-15T23:50:28Z

python/cugraph/tests/dask/test_mg_louvain.py

+                             names=['src', 'dst', 'value'],
+                             dtype=['int32', 'int32', 'float32'])
+
+    dg = cugraph.DiGraph()


We should start loading undirected graphs into Graph structure instead of DiGraph at some point. Basically, if MG Louvain supports only undirected graphs passing a DiGraph should throw an error from a user perspective. Asking users to pass symmetric data in an asymmetric container is a stretch.

Agreed. I'll probably have to defer this to the next PR though.

afender · 2020-09-15T23:54:25Z

python/cugraph/tests/dask/test_mg_louvain.py

+    setFixtureParamNames(request, ["dataset"])
+    dataset = request.param
+
+    chunksize = dcg.get_chunksize(dataset)


That won't fly for the targeted MG scale but ok for small smoke tests.

That won't fly for the targeted MG scale but ok for small smoke tests.

CI is not for scale testing. So not hitting the targeted MG scale in this test is fine

afender · 2020-09-15T23:57:51Z

python/cugraph/dask/community/louvain.py

+    """
+    Compute the modularity optimizing partition of the input graph using the
+    Louvain method on multiple GPUs
+


All public API parameters need to be documented

Agreed, that was an oversight on my part. I'll try to do that in the next commit to address Chuck's feedback, or may defer to the next PR.

afender · 2020-09-16T00:04:19Z

python/cugraph/dask/community/louvain.py

+    # from cugraph.structure.graph import Graph
+
+    # FIXME: dask methods to populate graphs from edgelists are only present on
+    # DiGraph classes. Disable the Graph check for now and assume inputs are


Why are we exposing symmetric DiGraph and throw errors for Graph or actual DiGraph? It is a bit awkward from a UX perspective since cuGraph has the Graph class for undirected graphs.

Agreed, there's still some tech debt there that I may have to address in the next PR.

BradReesWork · 2020-09-16T00:26:01Z

rerun tests

…ad of a value which gets copied.

… yet enabled.

rlratzel · 2020-09-16T08:05:01Z

rerun tests

BradReesWork · 2020-09-16T12:56:02Z

rerun tests

cpp/include/utilities/cython.hpp

…, added documentation, added deleted copy and assignment operators, added more legacy graph types for future expansion during the transition.

rlratzel added 5 commits September 9, 2020 23:05

safety commit - building with new MG Louvain placeholders

b0a5eff

Renamed graph_new to graph_primtypes to hopefully clarify its content…

4fccf69

…s, removed unused graph_new.pxd (which was identical to graph.pxd, which then got renamed to graph_primtypes.pxd)

WIP for calling MG Louvain from Cython wrapper, calling Dask correctl…

1c3cefa

…y, etc.

Added FIXME to test to describe an additional check that's needed

3cadbbb

Temp fix for a circular import problem

e2b8a58

rlratzel requested a review from a team as a code owner September 10, 2020 07:18

rlratzel self-assigned this Sep 10, 2020

rlratzel added the 2 - In Progress label Sep 10, 2020

Further updates to Louvain MG cython/wrapper code. No longer builds a…

175be02

…t the moment since it's not calling the Louvain C++ API which has not been merged yet.

afender reviewed Sep 10, 2020

View reviewed changes

rlratzel added 5 commits September 10, 2020 16:02

Merge remote-tracking branch 'upstream/branch-0.16' into branch-0.16-…

7c3d735

…mglouvaintesting2

Merge remote-tracking branch 'upstream/branch-0.16' into branch-0.16-…

43e9c86

…mglouvaintesting2

Updated to properly call new Louvain C++ API from cython wrapper, tes…

4f67ece

…t seems to be running and returning expected results now (based on comparison to non-dask SG test).

Added C++ graph container factory function and changed Louvain wrappe…

35e1524

…rs to call it.

Added PR 1139 to CHANGELOG.md

0630aae

rlratzel requested review from a team as code owners September 14, 2020 17:43

rlratzel changed the title ~~[WIP][skip-ci] MNMG Louvain Python updates, Cython cleanup~~ [REVIEW] MNMG Louvain Python updates, Cython cleanup Sep 14, 2020

rlratzel added 3 - Ready for Review and removed 2 - In Progress labels Sep 15, 2020

rlratzel requested review from ChuckHastings and Iroy30 September 15, 2020 16:09

ChuckHastings approved these changes Sep 15, 2020

View reviewed changes

aschaffer requested changes Sep 15, 2020

View reviewed changes

cpp/include/utilities/cython.hpp Outdated Show resolved Hide resolved

cpp/include/utilities/cython.hpp Outdated Show resolved Hide resolved

cpp/src/utilities/cython.cpp Outdated Show resolved Hide resolved

BradReesWork added 4 - Waiting on Author labels Sep 15, 2020

rlratzel added 2 commits September 15, 2020 16:43

Updates from review feedback: added/updated FIXMEs, using a proper sc…

628b828

…oped enum, removed dead code, removed unneeded std::move()

Merge remote-tracking branch 'upstream/branch-0.16' into branch-0.16-…

048178e

…mglouvaintesting2

aschaffer approved these changes Sep 15, 2020

View reviewed changes

rlratzel removed the 4 - Waiting on Author label Sep 15, 2020

afender approved these changes Sep 16, 2020

View reviewed changes

rlratzel added 4 commits September 15, 2020 23:55

Chnaged union to hold pointers, but having problem with a double free.

9054855

Fixed double free by passing a reference to a graph_container_t inste…

71aac7b

…ad of a value which gets copied.

Started adding new graph_t class instantiations, not done yet and not…

915a56e

… yet enabled.

C++ style fixes

427da31

rlratzel requested review from ChuckHastings and aschaffer September 16, 2020 07:52

rlratzel mentioned this pull request Sep 16, 2020

[WIP][skip-ci] Finalize adding support for new 2D graph class in Cython utils, address remaining feedback from 1139 #1146

Closed

ChuckHastings reviewed Sep 16, 2020

View reviewed changes

BradReesWork approved these changes Sep 16, 2020

View reviewed changes

rlratzel added 3 commits September 16, 2020 15:12

Addressed review feedback: passing const references for the container…

50446dd

…, added documentation, added deleted copy and assignment operators, added more legacy graph types for future expansion during the transition.

C++ style check fix

7228ab2

Fixed description of local_* parameters.

82d2a3d

BradReesWork merged commit 05d97a5 into rapidsai:branch-0.16 Sep 17, 2020

rlratzel deleted the branch-0.16-mglouvaintesting2 branch December 9, 2020 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] MNMG Louvain Python updates, Cython cleanup #1139

[REVIEW] MNMG Louvain Python updates, Cython cleanup #1139

rlratzel commented Sep 10, 2020 •

edited

Loading

GPUtester commented Sep 10, 2020

afender left a comment

rlratzel commented Sep 10, 2020

afender commented Sep 10, 2020

rlratzel commented Sep 10, 2020

rlratzel commented Sep 15, 2020

ChuckHastings left a comment

codecov-commenter commented Sep 15, 2020 •

edited

Loading

afender left a comment

afender Sep 15, 2020

rlratzel Sep 16, 2020

afender Sep 15, 2020

BradReesWork Sep 16, 2020

afender Sep 15, 2020

rlratzel Sep 16, 2020

afender Sep 16, 2020

rlratzel Sep 16, 2020

BradReesWork commented Sep 16, 2020

rlratzel commented Sep 16, 2020

BradReesWork commented Sep 16, 2020

[REVIEW] MNMG Louvain Python updates, Cython cleanup #1139

[REVIEW] MNMG Louvain Python updates, Cython cleanup #1139

Conversation

rlratzel commented Sep 10, 2020 • edited Loading

GPUtester commented Sep 10, 2020

afender left a comment

Choose a reason for hiding this comment

rlratzel commented Sep 10, 2020

afender commented Sep 10, 2020

rlratzel commented Sep 10, 2020

rlratzel commented Sep 15, 2020

ChuckHastings left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 15, 2020 • edited Loading

Codecov Report

afender left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradReesWork commented Sep 16, 2020

rlratzel commented Sep 16, 2020

BradReesWork commented Sep 16, 2020

rlratzel commented Sep 10, 2020 •

edited

Loading

codecov-commenter commented Sep 15, 2020 •

edited

Loading