-
Notifications
You must be signed in to change notification settings - Fork 72
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
fix library name in docs ('cuvs' not 'pycuvs') #193
fix library name in docs ('cuvs' not 'pycuvs') #193
Conversation
README.md
Outdated
@@ -36,19 +36,21 @@ cuVS comes with pre-built packages that can be installed through [conda](https:/ | |||
|
|||
| Python | C++ | C | | |||
|--------|-----|---| | |||
| `pycuvs`| `libcuvs` | `libcuvs_c` | | |||
| `cuvs`| `libcuvs` | `libcuvs_c` | |
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.
I don't think there is a libcuvs_c
package, but there is a libcuvs-static
: https://github.com/rapidsai/cuvs/blob/branch-24.08/conda/recipes/libcuvs/meta.yaml#L18
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.
There is a libcuvs_c
shared library, with a C API over libcuvs
:
Line 556 in 9dc3a4d
add_library( |
Lines 588 to 592 in 9dc3a4d
target_link_libraries( | |
cuvs_c | |
PUBLIC cuvs::cuvs ${CUVS_CTK_MATH_DEPENDENCIES} | |
PRIVATE raft::raft | |
) |
But you're right, it doesn't appear there's a conda package being distributed under that name.
I just pushed a commit updating the relevant docs, but maybe we should get another review from @benfred or @cjnolet to be sure the docs are representing these packages the way they'd like.
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.
I don't think there would be much point of a libcuvs_c
, since it just contains the C-API that requires lubcuvs
, so this change makes sense to me. Would like confirmation from @benfred that I'm not misremembering something.
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.
changes lgtm
/merge |
The Python library is distributed as conda package
cuvs
, but the installation docs saypycuvs
. This fixes that.Looked for other uses like this:
Didn't find any.