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

Support for polars 1.12 in cudf-polars #17227

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Oct 31, 2024

Description

No new updates are required, we must just no longer xfail a test if running with 1.12

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

pylibcudf is the known-rapids package we use, not cudf.
@wence- wence- requested review from a team as code owners October 31, 2024 18:42
@github-actions github-actions bot added Python Affects Python cuDF API. cudf.polars Issues specific to cudf.polars labels Oct 31, 2024
@wence- wence- added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 31, 2024
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I guess we forgot to update the ruff isort when we moved from a cudf to a pylibcudf dependency? Those changes cluttered this PR a bit as a result, but otherwise everything LGTM.

Should we start thinking about a world where we can support a range of polars versions, or are we not there yet? Probably need to upstream more IR testing first I suppose.

@wence-
Copy link
Contributor Author

wence- commented Oct 31, 2024

This one now supports two versions!

@vyasr
Copy link
Contributor

vyasr commented Oct 31, 2024

Yes! Sorry I should have been more clear, I'm asking if you think that this is a one-off or if you think we're getting to a place where we only need to bump the upper bounds (and eventually can remove the upper bound). IR stability is part of the equation, but perhaps not all of it since we also need to pass the polars test suite so behavior changes could hit us even if the IR is stable.

@wence-
Copy link
Contributor Author

wence- commented Oct 31, 2024

/merge

@rapids-bot rapids-bot bot merged commit 0929115 into rapidsai:branch-24.12 Oct 31, 2024
105 checks passed
@wence- wence- deleted the wence/fea/polars-1.12 branch October 31, 2024 21:53
@wence-
Copy link
Contributor Author

wence- commented Nov 1, 2024

Yes! Sorry I should have been more clear, I'm asking if you think that this is a one-off or if you think we're getting to a place where we only need to bump the upper bounds (and eventually can remove the upper bound). IR stability is part of the equation, but perhaps not all of it since we also need to pass the polars test suite so behavior changes could hit us even if the IR is stable.

Oh, I see what you mean.

I expect that we will continue to bump lower bounds as well, because although we can adapt to changes across versions in the IR (we can recognise this via IR versioning as well), I'd rather not do too much, and as you note, testing against all the versions might result in different answers due to bugs. As noticed here, I had a test that I expected to fail against 1.11 (due to a polars issue) that then needs to be removed for 1.12 (because it was fixed).

I suspect we will get to a point that is similar to the pandas situation: we have some lower bound that we periodically update. As discussed for other reasons, I think we should always have an upper bound, but it might get to the point where it is bounded above by the next major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf.polars Issues specific to cudf.polars improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants