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

[review] Open source nvgraph in cugraph repository #194

Merged
merged 19 commits into from
Apr 5, 2019

Conversation

afender
Copy link
Member

@afender afender commented Mar 29, 2019

This is the PR for #33

Contains:

  • Copy nvgraph code in cugraph repo under cpp/nvgraph
  • Integrate recent changes from perforce and minor cleanup after reviews
  • Add a path to produce nvgraph library only
  • Add a path to automatically build nvpraph code when building cugraph
  • rename nvgraph stand-alone library nvgraph_st into nvgraph_rapids
  • Reduce nvgraph build time by disabling undocumented features when built from cugraph (got 3x speedup)
  • update readme(s)

@afender afender changed the title Open source nvgraph in cugraph repository [wip] Open source nvgraph in cugraph repository Mar 29, 2019
@afender
Copy link
Member Author

afender commented Mar 29, 2019

Hi @raydouglass ,

I can repro the error I see on Jenkins only when I get CUB submodule with the command: git submodule update --init --recursive --remote cpp/nvgraph/cpp/thirdparty/cub .

It works fine when I get cub using git submodule update --init --recursive or even git clone --recurse-submodules [email protected]:afender/cugraph.git.

That sounds like an easy fix we could do on Jenkins, correct?

@afender afender requested a review from raydouglass April 1, 2019 16:57
@raydouglass
Copy link
Member

Hi @raydouglass ,

I can repro the error I see on Jenkins only when I get CUB submodule with the command: git submodule update --init --recursive --remote cpp/nvgraph/cpp/thirdparty/cub .

It works fine when I get cub using git submodule update --init --recursive or even git clone --recurse-submodules [email protected]:afender/cugraph.git.

That sounds like an easy fix we could do on Jenkins, correct?

Unfortunately not. Jenkins uses the former method to update submodules.

You should be able to resolve this by add branch information to .gitmodules like cuDF: https://github.com/rapidsai/cudf/blob/branch-0.7/.gitmodules
You likely want to use the same cub repository and branch as cuDF.

@afender
Copy link
Member Author

afender commented Apr 2, 2019

You should be able to resolve this by add branch information to .gitmodules

Thanks, I added branches to submodules and C++ builds pass now.

However, we have another problem on Jenkins for the python part. Cugraph now builds and installs nvgraph (under ${CMAKE_INSTALL_PREFIX}) but the python setup cannot find it on Jenkins later on (it searches in CONDA_PREFIX/lib and LD_LIBRARY_PATH).

@raydouglass how should we update the setup.py so that it can find libnvgraph_rapids.so on Jenkins?

@afender afender added the Blocked Cannot progress due to external reasons label Apr 2, 2019
@afender
Copy link
Member Author

afender commented Apr 4, 2019

@raydouglass ⬆️

@raydouglass
Copy link
Member

@afender I opened a PR against your branch: afender#8
I think that will resolve the problem.

FIX Separate building from uploading decision
@afender afender added 5 - Ready to Merge and removed Blocked Cannot progress due to external reasons labels Apr 4, 2019
Copy link
Member

@raydouglass raydouglass left a comment

Choose a reason for hiding this comment

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

LGTM

@afender afender changed the title [wip] Open source nvgraph in cugraph repository [review] Open source nvgraph in cugraph repository Apr 4, 2019
@BradReesWork BradReesWork merged commit c75e9d7 into rapidsai:branch-0.7 Apr 5, 2019
@afender
Copy link
Member Author

afender commented Apr 25, 2019

@raydouglass I was just talking to @rlratzel about conda packaging changes resulting from this PR in 0.7 release. I lost track if the updates have been done or not. There are basically two things I wanted to double check with you:

  1. We should keep releasing nvgraph as a requirement. We should just make sure that conda install nvgraph reflects what's in https://github.com/rapidsai/cugraph/tree/branch-0.7/cpp/nvgraph rather than https://gitlab-master.nvidia.com/RAPIDS/nvgraph.

  2. Now that nvgraph lives inside cugraph and is built along with it, should we update the way the cugraph conda packadge is built in 0.7? I suspect there is something to do so that conda install cugraph also installs nvgraph lib and headers?

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.

3 participants