Skip to content

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
@divyegala divyegala requested a review from raydouglass 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

jameslamb commented Oct 16, 2024

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-test.

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

@divyegala
Copy link
Member Author

@jameslamb yes, appreciate your suggestion here. I can do a quick follow on to add the minimal test and that will act as a placeholder in CI until the full test suite is ready.

@divyegala
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit c86e74d into rapidsai:branch-24.12 Oct 17, 2024
55 checks passed
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