[WIP] Expose cumlHandle (+ other goodies!) into cython world #331

teju85 · 2019-03-14T06:07:32Z

This PR exposes cumlHandle into the cython world, over-and-above @jirikraus's work in PR #247 .
Additionally, this also proposes for a Base class to be inherited by all ML algos. Such base class will go a long way in reducing code duplication across cuML python interface.

I've used PCA as an example to demonstrate the cumlHandle + Base class related updates. The hope is that all of us can use this as an example to update other algos too (including the new ones).

Things addressed in this PR:

Update the c++ interface of PCA to expose cumlHandle
Join the cython and c++ interfaces after this change
Add a developer guide from the python side
Expose setAllocator methods for the cumlHandle class into cython world
For all the affected methods (only which PCA calls today!), show how to use deviceAllocator instead of the existing 'DeviceAllocator` class.

…n cython

…een Stream and Handle classes

…scikit-esq ML classes

…etup Handle

…a-ext-cython-cumlhandle

…keywords inside sgd.pyx

jirikraus

I can't really provide feedback on the python side of this. I hope my comments are still useful.

python/cuml/_common/base.pyx

python/cuml/_common/handle.pyx

…a-ext-cython-cumlhandle

…mature now

…le of cython build errors

GPUtester · 2019-03-19T04:32:56Z

Can one of the admins verify this patch?

…a-ext-cython-cumlhandle

… PCA

teju85 · 2019-03-19T10:22:03Z

@jirikraus I'd like to particularly bring your attention to my previous commit #000176b. I have migrated the default*Allocator classes inside of ml-prims! Reason being, I'd like to deprecate the existing DeviceAllocator class inside ml-prims and use defaultDeviceAllocator inside unit-tests of ml-prims. Do you have any concerns with this migration?

…n calls between them

jirikraus · 2019-03-19T13:57:17Z

@jirikraus I'd like to particularly bring your attention to my previous commit #000176b. I have migrated the default*Allocator classes inside of ml-prims! Reason being, I'd like to deprecate the existing DeviceAllocator class inside ml-prims and use defaultDeviceAllocator inside unit-tests of ml-prims. Do you have any concerns with this migration?

Absolutely not. I thought that was the intention of moving the abstract interface of ml-prims.

P.S. FYI: I would like to take a more detailed look on the C++ things you added, but I am not sure if I find time for it this week while I am at GTC. One thought I had after giving this a quick view: I think it would make sense to separate out the basic infrastructure changes from applying it to the pca and tsvd algorithms in different PRs. I understand that you need an algorithm to try this, but I think the review would be more efficient if it is done desperately. Does that make sense to you?

jirikraus · 2019-03-27T11:28:57Z

unfortunately not. There are parts of tsvd that depend on pca. So, if I change pca that'll affect tsvd too. This PR is looking big because I got carried away with the device-allocator thingy too. Just the cumlHandle changes will probably create a much smaller PR. I'm currently doing the same in my local branch.

Not sure if you got my point. If tsvd depends on pca: Why not use PCA only for the first PR and after that is merged target tsvd. Is there also a depedency of PCA to TSVD? Or perhaps I do not see the reason to have two examples in the initial PR.

teju85 · 2019-03-27T11:41:01Z

My bad. I actually meant "pca depends on tsvd".

never mind my previous comments. I found a work-around to just confine the changes to PCA alone.

cuML/src/glm/ols.h

jirikraus · 2019-04-01T14:41:28Z

cuML/src/pca/pca.h

+    truncCompExpVars(handle, cov.data(), components, explained_var,
+                     explained_var_ratio, prms);
+    math_t scalar = (prms.n_rows - 1);
+    Matrix::seqRoot(explained_var, singular_vals, scalar, prms.n_components, true);


No stream or handle?

jirikraus · 2019-04-01T14:41:40Z

cuML/src/pca/pca.h

+                     explained_var_ratio, prms);
+    math_t scalar = (prms.n_rows - 1);
+    Matrix::seqRoot(explained_var, singular_vals, scalar, prms.n_components, true);
+    Stats::meanAdd(input, input, mu, prms.n_cols, prms.n_rows, false, true);


No stream or handle?

jirikraus · 2019-04-01T14:45:21Z

cuML/src/pca/pca.h

+    pcaFit(handle, input, components, explained_var, explained_var_ratio, singular_vals,
+           mu, noise_vars, prms);
+    pcaTransform(handle, input, components, trans_input, singular_vals, mu, prms);
+    signFlip(trans_input, prms.n_rows, prms.n_components, components,


No stream or handle passed in. I see more instances of this below but will not call them all out.

jirikraus · 2019-04-01T14:49:19Z

cuML/src/tsvd/tsvd.h

+    Stats::sum(total_vars.data(), vars.data(), 1, prms.n_cols, false);
+
+    math_t total_vars_h;
+    updateHost(&total_vars_h, total_vars.data(), 1);


synchronous updateHost does not take a stream. This needs to be

updateHostAsync( ..., stream); cudaStreamSynchronize(stream);

if I do not miss anything.

jirikraus · 2019-04-01T14:51:27Z

ml-prims/src/linalg/eig.h

  CUDA_CHECK(cudaGetLastError());

  int dev_info;
-  updateHost(&dev_info, d_dev_info, 1);
+  updateHost(&dev_info, d_dev_info.data(), 1);


To ensure correct stream ordering this needs to be updateHostAsync + cudaStreamSynchronize if I do not miss anything.

…a-ext-cython-cumlhandle

…nt ml-prims calls to pass stream. Still need to update Matrix:: namespace to accept stream and updateHost to be async

teju85 · 2019-04-02T11:21:48Z

@dantegd the build is failing in CI and that's mostly due to the setup.py changes in this commit are not being picked up during the build!?

…reprocess.h accordingly

kkraus14 · 2019-04-02T18:23:59Z

@dantegd the build is failing in CI and that's mostly due to the setup.py changes in this commit are not being picked up during the build!?

The pip build looks for setup_pip.py instead of setup.py. Both need to be updated.

…a-ext-cython-cumlhandle

teju85 · 2019-04-03T05:54:57Z

Thanks @kkraus14

… all function calls

teju85 · 2019-04-07T03:54:57Z

Hi all,
Myself and @jirikraus discussed about this PR offline. Here's the summary:

Let Vinay's PR for dbscan+cumlHandle merged first (Introducing cumlHandle API to dbscan and add example #394)
Then get my new PR which only exposes cython-cumlHandle merged ([REVIEW] Exposing cumlhandle in cython #435)
Then create another PR to link the changes in 1 and 2 together
Get the remaining C++ changes I have created in this PR file as a separate one

@dantegd you'll have to (re-)review the python changes once more in PR #435 ! Sorry about the extra-work that this has created for you.

cjnolet · 2019-04-16T14:59:51Z

Based on this comment:

Get the remaining C++ changes I have created in this PR file as a separate one

@teju85 @jirikraus Can we close this PR?

jirikraus · 2019-04-16T15:13:37Z

Based on this comment:

Get the remaining C++ changes I have created in this PR file as a separate one

@teju85 @jirikraus Can we close this PR?

@teju85 has the final call, but I think yes this PR is outdated.

teju85 · 2019-04-16T17:10:19Z

There's only one item pending from my previously commented list above, and that is the corresponding cumlHandle updates for pca and tsvd algos. I have those changes being prepared in pipeline.

Will close this PR, once that is filed. Give me some more time.

teju85 · 2019-04-16T17:49:26Z

PR #482 filed for the pca + tsvd related code cleanup and cumlHandle exposure. This PR is no longer needed. Closing.

teju85 added 7 commits March 12, 2019 02:26

Initial failed experimentations with wrapping cumlHandle!

66a7dc0

Checking in the current state of my efforts for wrapping cumlHandle i…

34d1bd0

…n cython

Fixed the issues with trying to access the underlying cudaStream betw…

38cbce2

…een Stream and Handle classes

moved the common folder and added a base class to be used by for all …

f7c03fb

…scikit-esq ML classes

Used PCA as an example to show how to use the Base class as well as s…

75aeaad

…etup Handle

Merge branch 'branch-0.7' of https://github.com/rapidsai/cuml into fe…

e5143ca

…a-ext-cython-cumlhandle

updated changelog

29c7be1

dantegd added 2 - In Progress Currenty a work in progress proposal Change current process or code Cython / Python Cython or Python issue labels Mar 14, 2019

renamed the _common to common for simplicity. Added our usual cython …

d35be6c

…keywords inside sgd.pyx

jirikraus reviewed Mar 14, 2019

View reviewed changes

python/cuml/_common/base.pyx Outdated Show resolved Hide resolved

python/cuml/_common/handle.pyx Outdated Show resolved Hide resolved

teju85 added 4 commits March 14, 2019 04:00

fixed an issue with test utils

8cbedbc

Merge branch 'branch-0.7' of https://github.com/rapidsai/cuml into fe…

014a441

…a-ext-cython-cumlhandle

removed random_state param from base class, as this idea is a bit pre…

1ec99ff

…mature now

added an initial version of developer guide

4b1d034

teju85 mentioned this pull request Mar 15, 2019

[FEA] Add option to use RMM in cuML #324

Closed

teju85 added 4 commits March 17, 2019 20:40

Initial set of changes to link rmm build from cudf

376d158

added an example of how to enable RMM in the documentation

ffaa5ef

cumlHandle related changes in the C++ side as well. Also fixed a coup…

0b8286d

…le of cython build errors

first successful pass of python build and unit-tests after enabling RMM

01e9c2c

teju85 added 3 commits March 18, 2019 21:51

Merge branch 'branch-0.7' of https://github.com/rapidsai/cuml into fe…

af40ebc

…a-ext-cython-cumlhandle

removal cublasCreate and cusolverCreate calls

82857d5

hooked up allocator offered by cumlHandle to allocate temp buffers in…

000176b

… PCA

teju85 added 2 commits March 19, 2019 04:48

attempt at cleaning tsvd as well, since pca and tsvd share some commo…

1d5b75f

…n calls between them

updated pcaInverseTransform to use cumlHandle

75abf5e

This was referenced Mar 27, 2019

Exposing cumlHandle in the cuML interface and corresponding cython changes #396

Closed

Introducing cumlHandle API to dbscan and add example #394

Merged

jirikraus reviewed Apr 1, 2019

View reviewed changes

teju85 added 5 commits April 2, 2019 02:18

Merge branch 'branch-0.7' of https://github.com/rapidsai/cuml into fe…

c422d2a

…a-ext-cython-cumlhandle

fixed a compilation issue during the previous merge commit

f8d08ec

took care of developer guide comments

a1d1559

incorporated suggestions by Dante

b0d06b2

Cleaned up the internal calls of OLS to use cumlhandle and the releva…

3b89b21

…nt ml-prims calls to pass stream. Still need to update Matrix:: namespace to accept stream and updateHost to be async

updated matrix/math.h header to expose stream parameter and updated p…

c6e6005

…reprocess.h accordingly

teju85 added 2 commits April 2, 2019 22:43

Merge branch 'branch-0.7' of https://github.com/rapidsai/cuml into fe…

09a98d1

…a-ext-cython-cumlhandle

updated setup_pip.py with rmm related changes as well

6ce4ce0

teju85 added 3 commits April 2, 2019 23:30

checked and updated OLS code to use streams + handles properly across…

31fb14a

… all function calls

updated members under Matrix:: namespace to accept stream parameter

419ad48

updated ridge algo to also use stream and handle

76d7cae

teju85 mentioned this pull request Apr 3, 2019

[REVIEW] Exposing cumlhandle in cython #435

Merged

teju85 mentioned this pull request Apr 14, 2019

[REVIEW] Expose cumlhandle for dbscan in python #475

Merged

teju85 changed the title ~~[REVIEW] Expose cumlHandle (+ other goodies!) into cython world~~ [WIP] Expose cumlHandle (+ other goodies!) into cython world Apr 16, 2019

teju85 closed this Apr 16, 2019

teju85 deleted the fea-ext-cython-cumlhandle branch April 16, 2019 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Expose cumlHandle (+ other goodies!) into cython world #331

[WIP] Expose cumlHandle (+ other goodies!) into cython world #331

teju85 commented Mar 14, 2019 •

edited

Loading

jirikraus left a comment

GPUtester commented Mar 19, 2019

teju85 commented Mar 19, 2019

jirikraus commented Mar 19, 2019

jirikraus commented Mar 27, 2019

teju85 commented Mar 27, 2019

jirikraus Apr 1, 2019

jirikraus Apr 1, 2019

jirikraus Apr 1, 2019

jirikraus Apr 1, 2019

jirikraus Apr 1, 2019

teju85 commented Apr 2, 2019

kkraus14 commented Apr 2, 2019

teju85 commented Apr 3, 2019

teju85 commented Apr 7, 2019

cjnolet commented Apr 16, 2019

jirikraus commented Apr 16, 2019

teju85 commented Apr 16, 2019

teju85 commented Apr 16, 2019

[WIP] Expose cumlHandle (+ other goodies!) into cython world #331

[WIP] Expose cumlHandle (+ other goodies!) into cython world #331

Conversation

teju85 commented Mar 14, 2019 • edited Loading

jirikraus left a comment

Choose a reason for hiding this comment

GPUtester commented Mar 19, 2019

teju85 commented Mar 19, 2019

jirikraus commented Mar 19, 2019

jirikraus commented Mar 27, 2019

teju85 commented Mar 27, 2019

jirikraus Apr 1, 2019

Choose a reason for hiding this comment

jirikraus Apr 1, 2019

Choose a reason for hiding this comment

jirikraus Apr 1, 2019

Choose a reason for hiding this comment

jirikraus Apr 1, 2019

Choose a reason for hiding this comment

jirikraus Apr 1, 2019

Choose a reason for hiding this comment

teju85 commented Apr 2, 2019

kkraus14 commented Apr 2, 2019

teju85 commented Apr 3, 2019

teju85 commented Apr 7, 2019

cjnolet commented Apr 16, 2019

jirikraus commented Apr 16, 2019

teju85 commented Apr 16, 2019

teju85 commented Apr 16, 2019

teju85 commented Mar 14, 2019 •

edited

Loading