Go API - [WIP] #212

ajit283 · 2024-07-03T19:57:45Z

This PR adds a Go API. It's far from completion and still work in progress. Feel free to suggest improvements!

…nagedTensor function.

copy-pr-bot · 2024-07-03T19:57:48Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

cjnolet · 2024-07-09T21:36:39Z

/ok to test

cjnolet · 2024-11-15T22:05:05Z

go/brute_force/brute_force.go

@@ -0,0 +1,92 @@
+package brute_force
+
+// #include <cuda_runtime_api.h>


Can these be removed?

The Go compiler uses these comments to link the necessary cuvs source files: https://go.dev/wiki/cgo

cjnolet · 2024-11-15T22:05:14Z

go/brute_force/brute_force.go

+	trained bool
+}
+
+// func (index *Index) Close() {


This too- can this be removed?

cjnolet · 2024-11-15T22:08:41Z

go/cagra/index_params.go

+	return p, nil
+}
+
+func CreateIndexParams() (*IndexParams, error) {


Are these function names namespaced in some way? For example, will this conflict with an ivfpq CreateIndexParams or does the Go compiler know that this is different because it's nested in a cagra directory? I wonder if we should mirror the C nameing convention (i.e. cuvsCagraIndexParamsCreate, prefix functions with cuvs, followed by the subject and put the verb at the end).

Nevermind, I see the package cagra at the top of the file so I'm assuming everything is nested inside. I think I'd prefer to fully qualify these packages, though, for consistency with the other cuVS APIs: cuvs.neighbors.cagra.

cjnolet · 2024-11-15T22:08:59Z

go/cagra/index_params.go

+
+	// CBuildAlgo, exists := CBuildAlgos[build_algo]
+
+	// if !exists {


Please make sure to remove all commented out code.

cjnolet · 2024-11-15T22:09:37Z

go/cagra/index_params.go

+	AutoSelect: C.AUTO_SELECT,
+}
+
+func CreateCompressionParams() (*CompressionParams, error) {


Can we please document all of these functions? Does Go have a common convention or tooling for generating standardized documentation?

cjnolet · 2024-11-15T22:10:24Z

go/distance.go

+	DistanceDice:          C.DiceExpanded,
+}
+
+func PairwiseDistance[T any](Resources Resource, x *Tensor[T], y *Tensor[T], distances *Tensor[float32], metric Distance, metric_arg float32) error {


This is really great! I like how nicely this API translates.

cjnolet · 2024-11-15T22:11:13Z

go/distance.go

+// #include <cuda_runtime_api.h>
+// #include <cuvs/core/c_api.h>
+// #include <cuvs/distance/pairwise_distance.h>
+// #include <cuvs/neighbors/brute_force.h>


For ease of maintenance, can we please create a separate directory for all the testing code?

In Go, the convention is to have the tests in the same folder, but with a _test suffix, see here: https://blog.jetbrains.com/go/2022/11/22/comprehensive-guide-to-testing-in-go/

The Go toolchain recognizes these files as tests and also excludes them from the compiled binary.

cjnolet · 2024-11-15T22:12:03Z

go/cagra/search_params.go

@@ -0,0 +1,160 @@
+package cagra


For consistency, I think I'd prefer to mirror the top-level namspaces also (e.g. cuvs/neighbors/cagra. Can we do nested packages like this?

cjnolet · 2024-11-15T22:14:27Z

go/dlpack.go

@@ -0,0 +1,464 @@
+package cuvs
+
+// #include <stdio.h>


This would be easier to follow if we mirrored the existing cuVS directory structures for C/C++: cuvs/core/dlpack.go, etc...

I was orienting myself at the rust library: https://github.com/rapidsai/cuvs/tree/branch-24.12/rust/cuvs/src
We can still put in a seperate folder though, I'm fine with either way

cjnolet · 2024-11-15T22:19:03Z

@ajit283 this is a great contribution and very welcomed! Overall, I think this PR is shaping up well. Each API needs documentation, and ideally usage examples. It would also be ideal to build the documentation as part of this PR. We use Sphinx for our documentation (with a plugin for Doxygen so that it can include our C++ docs alongside our Python docs). I believe there's a Go plugin for Sphinx also.

Please remove commented out code and structure your files to mirror C/C++ codebase and namespaces as much as possible.

I'm really happy to get this in 24.12! JFYI, burndown starts next Thursday (Nov 21) and code freeze is the following Thursday (you can learn more at https://docs.rapids.ai/maintainers/).

cjnolet · 2024-11-20T22:17:32Z

/ok to test

ajit283 · 2024-11-22T12:05:04Z

ci/build_go.sh

+
+rapids-dependency-file-generator \
+  --output conda \
+  --file-key go \


do you know how I could add this file key to rapids-dependency-file-generator?

cc @bdice @vyasr @AyodeAwe any suggestions here?

Add a key under files: in dependencies.yaml, then add dependency lists with the conda packages you need. Here’s the rust one for comparison:

cuvs/dependencies.yaml

Line 70 in 710e9f5

rust:

More explanation in the README here: https://github.com/rapidsai/dependency-file-generator

ajit283 · 2024-11-22T12:11:48Z

.github/workflows/build.yaml

+  go-build:
+    needs: cpp-build
+    secrets: inherit
+    uses: rapidsai/shared-workflows/.github/workflows/[email protected]


am I doing this correctly?

ajit283 and others added 14 commits June 14, 2024 15:18

wip

7961798

cleanup

764dc1e

Added support for different data types and device allocation in NewMa…

b0fd0b1

…nagedTensor function.

bruteforce works

b47cf5b

refactor

1ec0c8c

improve resources

d5c0dc3

add ivf_flat, modules not working yet

cc6319e

sync

01af7c9

cleanup

ed9bf47

Merge branch 'rapidsai:branch-24.08' into go

4e79d8e

rename

d278abc

sync

b91721d

.

6b90861

Merge branch 'rapidsai:branch-24.08' into go

773fd94

ajit283 requested a review from a team as a code owner July 3, 2024 19:57

github-actions bot added the CMake label Jul 3, 2024

benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 3, 2024

cjnolet assigned ajit283 Jul 9, 2024

ajit283 and others added 5 commits July 10, 2024 13:09

sync

7cbc1f9

ivf_pq + cagra

46ec2f7

Merge branch 'branch-24.08' into go

9dcffbd

CI/CD attempt

f82216c

add distance

486d40a

ajit283 requested review from a team as code owners July 10, 2024 17:26

ajit283 requested a review from jameslamb July 10, 2024 17:26

github-actions bot added the ci label Jul 10, 2024

ajit283 and others added 4 commits October 16, 2024 19:35

add memory resource test

04b6532

thread_local fix

a082f67

change pool memory

a84e764

Merge branch 'branch-24.10' into go

853d538

cjnolet changed the base branch from branch-24.10 to branch-24.12 November 4, 2024 20:20

cjnolet and others added 3 commits November 4, 2024 15:20

Merge branch 'branch-24.12' into go

4021229

simplify memory_resource

6dd2044

Merge branch 'go' of https://github.com/ajit283/cuvs into go

3e33692

cjnolet reviewed Nov 15, 2024

View reviewed changes

go/brute_force/brute_force.go Outdated

trained bool

}

// func (index *Index) Close() {

Copy link

Member

cjnolet Nov 15, 2024

Choose a reason for hiding this comment

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

This too- can this be removed?

cjnolet reviewed Nov 15, 2024

View reviewed changes

Merge branch 'branch-24.12' into go

b1a0476

ajit283 and others added 5 commits November 21, 2024 22:40

Merge branch 'branch-24.12' into go

dddb165

cleanup top-level packages

e05782a

cleanup neighbors

a315632

.

3c97864

build

bd2dd76

ajit283 commented Nov 22, 2024

View reviewed changes

ci

bd76cf8

ajit283 commented Nov 22, 2024

View reviewed changes

ajit283 added 2 commits November 22, 2024 18:39

ci

1e5a756

fix pointer pinning issues

927f2be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go API - [WIP] #212

Go API - [WIP] #212

ajit283 commented Jul 3, 2024

copy-pr-bot bot commented Jul 3, 2024

cjnolet commented Jul 9, 2024

cjnolet Nov 15, 2024

ajit283 Nov 21, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

ajit283 Nov 21, 2024

cjnolet Nov 15, 2024

cjnolet Nov 15, 2024

ajit283 Nov 21, 2024

cjnolet commented Nov 15, 2024

cjnolet commented Nov 20, 2024

ajit283 Nov 22, 2024

cjnolet Nov 22, 2024

bdice Nov 22, 2024

bdice Nov 22, 2024

ajit283 Nov 22, 2024

		@@ -0,0 +1,92 @@
		package brute_force

		// #include <cuda_runtime_api.h>


		// CBuildAlgo, exists := CBuildAlgos[build_algo]

		// if !exists {

Go API - [WIP] #212

Are you sure you want to change the base?

Go API - [WIP] #212

Conversation

ajit283 commented Jul 3, 2024

copy-pr-bot bot commented Jul 3, 2024

cjnolet commented Jul 9, 2024

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

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

cjnolet commented Nov 15, 2024

cjnolet commented Nov 20, 2024

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