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

Add click package to cuvs-bench conda recipe #408

Open
wants to merge 1 commit into
base: branch-24.12
Choose a base branch
from

Conversation

divyegala
Copy link
Member

This package is available in dependencies.yaml, but due to an oversight was not added to conda metas.

@divyegala divyegala added bug Something isn't working non-breaking Introduces a non-breaking change labels Oct 10, 2024
@divyegala divyegala self-assigned this Oct 10, 2024
@divyegala divyegala requested a review from a team as a code owner October 10, 2024 21:02
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Approving this so you can merge this if it's time-sensitive, but I do have a suggestion.

Seeing a run-time dependency missed made me think "how could this not have been caught by tests"? And it looks like the answer is "there aren't any tests run in CI".

Would you please consider adding some tests on this package in https://github.com/rapidsai/cuvs/blob/branch-24.12/ci/test_python.sh?

I know this is a little different than other libraries because it's for benchmarking, but you could for example add a test like this one that all the other RAPIDS libraries have:

https://github.com/rapidsai/raft/blob/907edb8261a94a06f8857d0a10f2ec9abcd1ae42/python/raft-ann-bench/src/raft_ann_bench/_version.py

and then a test like this:

https://github.com/rapidsai/cuvs/blob/f62b217f97c9e14b340f11bcbfe556fcad9ed816/python/cuvs/cuvs/test/test_version.py

That's be a cheap and lightweight way to at least minimally test that the package is installable and importable. It might not have caught this specific issue because click is probably somewhere in the dependency tree of other packages, but it would catch this type of problem for other dependencies, and give us higher confidence in releases.

@dantegd
Copy link
Member

dantegd commented Oct 16, 2024

@jameslamb fully agree with your comment, adding tests is in the backlog and something I've wanted to get to as bandwidth permits. I even added unit code pytests, but haven't had time to fully check them and enable them, it something we plan to do as soon as we can alongside as benchmarks like you suggest in a follow up :)

@divyegala
Copy link
Member Author

@jameslamb we have a test suite that we'll be focussing on getting ready this release https://github.com/rapidsai/cuvs/tree/branch-24.12/python/cuvs_bench/cuvs_bench/tests. Until then, if it's okay with you, let's merge this PR.

@jameslamb
Copy link
Member

if it's okay with you, let's merge this PR.

Yeah totally fine.

we have a test suite that we'll be focussing on getting ready this release

Sure, I understand. I think it'd be worth adding something lightweight like the test_version.py I mentioned in #408 (review) sooner (instead of waiting to add the full test suite you want). Having some minimal validation that the conda packages are at least installable and importable would be useful.

You're not getting that kind of coverage from conda mambabuild because all the conda builds here are done with --no-tests.

cuvs/ci/build_python.sh

Lines 35 to 39 in f62b217

rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \
conda/recipes/cuvs_bench

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants