Skip to content

CI Fix tests to be compatible with new stricter cudf dtype handling#7762

Merged
rapids-bot[bot] merged 1 commit intorapidsai:mainfrom
betatim:fix-cudf-dtype-conversion
Feb 4, 2026
Merged

CI Fix tests to be compatible with new stricter cudf dtype handling#7762
rapids-bot[bot] merged 1 commit intorapidsai:mainfrom
betatim:fix-cudf-dtype-conversion

Conversation

@betatim
Copy link
Copy Markdown
Member

@betatim betatim commented Feb 4, 2026

See rapidsai/cudf#21281 for cudf changes. This changed what cudf accepts as a valid dtype.

@betatim betatim requested a review from a team as a code owner February 4, 2026 14:26
@betatim betatim requested a review from jcrist February 4, 2026 14:26
@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Feb 4, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 4, 2026

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved string data type handling in metrics to prevent invalid casting operations and leverage automatic string type inference.

Walkthrough

The change updates cudf dtype casting in the convert function. When handling cudf Series with string dtype targets, the code now skips the astype("str") call and relies on cudf's automatic string dtype inference, while other dtypes proceed normally with astype casting.

Changes

Cohort / File(s) Summary
cudf dtype casting optimization
python/cuml/tests/test_metrics.py
Modified cudf Series dtype handling to avoid potentially invalid astype("str") call; string dtype targets now return Series directly using cudf's automatic inference.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: updating tests for compatibility with cudf's new stricter dtype handling.
Description check ✅ Passed The description is related to the changeset, providing context about cudf changes that necessitated the test updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@betatim betatim mentioned this pull request Feb 4, 2026
Copy link
Copy Markdown
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Are we sure that this was intentional? I'm asking, because pandas accepts ".astype("str").

@betatim
Copy link
Copy Markdown
Member Author

betatim commented Feb 4, 2026

I think yes, at least there is quite a bit of code that now checks is_dtype_obj_string (and others like it). This makes me think that they only want to accept dtype objects, not "str" anymore. Should they make this change? I don't know.

Maybe @mroeschke can tell us if my interpretation is right

@csadorf csadorf added bug Something isn't working non-breaking Non-breaking change labels Feb 4, 2026
Copy link
Copy Markdown
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Happy to merge to unblock, but I'm overall a bit skeptical of the quality of this test.

@csadorf
Copy link
Copy Markdown
Contributor

csadorf commented Feb 4, 2026

/merge

@rapids-bot rapids-bot Bot merged commit dffff24 into rapidsai:main Feb 4, 2026
199 of 201 checks passed
@jcrist
Copy link
Copy Markdown
Member

jcrist commented Feb 5, 2026

I'm a bit confused - the test only was failing on the cudf-pandas enabled runs, which suggests to me that enabling cudf-pandas changed the behavior of cudf itself in some way. This doesn't match my understanding of how cudf.pandas works, so I do wonder if this is a bit of a cudf/cudf.pandas bug rather than expected new behavior.

@betatim betatim deleted the fix-cudf-dtype-conversion branch February 5, 2026 08:21
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Feb 5, 2026

Yeah, I am not sure if this side effect was on purpose or not, or if a layer of conversion is now missing from cudf.pandas (eg cudf wants this and the corresponding changes in cudf.pandas got forgotten).

However, because something like this getting merged, in this case in cudf, immediately blocks all of our CI I only care a little bit about what the intention was/is. I'd like to be able to work on cuml while they sort out the cudf/cudf.pandas side. My preferred solution is frozen dependencies in the CI jobs so that we can be intentional about this instead of firefighting.

@mroeschke
Copy link
Copy Markdown
Contributor

Sorry, yes this is working around a cuDF bug probably caused by rapidsai/cudf#21281 and this change wasn't intentional.

For context, the cuDF Python team is currently working on making cudf and cudf.pandas have less divergent behaviors for the next release by removing mode.pandas_compatible flags internally which causes cudf.pandas to take different code paths than cudf. So this was a case where one flag was removed but needed another flag to also be removed to maintain behavior. Will comment again once I have a fix so you can remove this workaround

dantegd added a commit to dantegd/cuml that referenced this pull request Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants