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

[FEA] cudf_kafka: add unit tests #15841

Closed
jameslamb opened this issue May 23, 2024 · 3 comments · Fixed by #15853
Closed

[FEA] cudf_kafka: add unit tests #15841

jameslamb opened this issue May 23, 2024 · 3 comments · Fixed by #15853
Labels
feature request New feature or request

Comments

@jameslamb
Copy link
Member

jameslamb commented May 23, 2024

Is your feature request related to a problem? Please describe.

As far as I can tell (please correct me if I'm wrong):

  • cudf_kafka wheels are not currently tested in this project's CI.
  • the only testing of cudf_kafka conda packages is an import cudf_kafka run during the conda build:

test:
requires:
- cuda-version ={{ cuda_version }}
imports:
- cudf_kafka

Describe the solution you'd like

This project should test cudf_kafka functionality in its CI.

At a minimum, a unit test that just runs import cudf_kafka and checks something trivial about it would provide some coverage of concerns like "is the package syntactically-valid Python for each version of Python it claims to support" and might catch issues with linking to libcudf.

A simple tests like the ones proposed in #15245 would be sufficient to at least establish the pattern for where to add more tests.

This could look like:

import cudf_kafka

def test_version_constants_are_populated():
    # __git_commit__ will only be non-empty in a built distribution
    assert isinstance(cudf.__git_commit__, str)

    # __version__ should always be non-empty
    assert isinstance(cudf.__version__, str)
    assert len(cudf.__version__) > 0

Describe alternatives you've considered

I don't think this needs to go as far as #5571 to be valuable.

Another alternative is to just add more testing to the cudf_kafka directly in the conda build recipe... but I think having tests on the wheels is valuable, as it'll catch different things.

Additional context

Created this based on this conversation: #15245 (comment)

@jameslamb jameslamb added feature request New feature or request ci labels May 23, 2024
@bdice
Copy link
Contributor

bdice commented May 23, 2024

@jameslamb Last time this topic came up we concluded not to produce cudf_kafka wheels. There are very few consumers of this library, and at that point, the build system would have required us to build and ship all of libcudf as part of both the cudf-cu(11|12) and cudf_kafka-cu(11|12) wheel if we chose to produce one. Perhaps the new C++ wheels for libcudf would solve part of this problem (building and shipping less in the Python wheels) but we'd probably need libcudf_kafka wheels in addition to libcudf wheels. This would be pretty complicated for very little gain, given the low usage of this library.

I am inclined to add unit tests that are as minimal as possible for conda packages and not ship cudf_kafka wheels at all. That would split the scope of this issue: add unit tests 👍 , add wheels 👎.

References:

@jameslamb
Copy link
Member Author

Ah!!!

@bdice I didn't realize that we weren't shipping cudf_kafka wheels. I definitely did not mean for this issue to propose doing that! My fault for assuming and not checking more closely 🙈

I'll modify the title and description to reflect that this is only about adding tests for the conda package.

as minimal as possible

100%, my main motivation for this was wanting a test to confirm that cudf_kafka.__version__ is populated and available. I'm not proposing going any further than that.

@jameslamb jameslamb changed the title [FEA] cudf_kafka: add unit tests and CI testing wheels [FEA] cudf_kafka: add unit tests May 23, 2024
@vyasr
Copy link
Contributor

vyasr commented May 23, 2024

Agree with what Bradley said: yes to tests, no to wheels.

rapids-bot bot pushed a commit that referenced this issue May 29, 2024
Fixes #15841

Proposes adding a basic unit test setup for `cudf_kafka`.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #15853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants