Skip to content

Bump minimum Polars version to 1.35 - #22459

Merged
rapids-bot[bot] merged 10 commits into
NVIDIA:mainfrom
mroeschke:feat/cudf_polars/min_bump
May 12, 2026
Merged

Bump minimum Polars version to 1.35#22459
rapids-bot[bot] merged 10 commits into
NVIDIA:mainfrom
mroeschke:feat/cudf_polars/min_bump

Conversation

@mroeschke

@mroeschke mroeschke commented May 11, 2026

Copy link
Copy Markdown
Contributor

Description

Broken off from #22048

Updates the minimum Polars version in cudf_polars based on the minimum supported Polars version in cloud environments #22048 (comment)

Code changes are purely removals (with minor reorganizations). Commits are split by workarounds removed per version.

Additionally, shortens the timeout of each test run per Polars version to 15 minutes each from 1 hour. A "normal" test run per Polars version in CI should complete in 2-3 minutes so 15 minutes should hopefully be OK

Checklist

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

@mroeschke mroeschke self-assigned this May 11, 2026
@mroeschke
mroeschke requested a review from a team as a code owner May 11, 2026 19:11
@mroeschke mroeschke added the improvement Improvement / enhancement to an existing function label May 11, 2026
@mroeschke
mroeschke requested a review from a team as a code owner May 11, 2026 19:11
@mroeschke mroeschke added the breaking Breaking change label May 11, 2026
@github-actions github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels May 11, 2026
@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR raises the minimum supported Polars version from 1.30 to 1.35 across the cudf-polars project, tightens Polars pins in tooling and environment files, and removes numerous Polars-version-gated code paths across the DSL, expression handlers, containers, aggregation logic, and tests.

Changes

Minimum Version Bound and Dependency Updates

Layer / File(s) Summary
Version Constants
python/cudf_polars/cudf_polars/utils/versions.py
POLARS_LOWER_BOUND raised from 1.30 to 1.35; precomputed POLARS_VERSION_LT_* constants for versions <1.36 removed.
Dependency Specifications
python/cudf_polars/pyproject.toml, dependencies.yaml, .pre-commit-config.yaml, conda/environments/*, conda/recipes/cudf-polars/recipe.yaml
Polars version constraint tightened to >=1.35,<1.39 across project metadata, CI tooling, environment manifests, and recipe files.

Core Library Compatibility Cleanup

Layer / File(s) Summary
IR Translation and Version Flags
python/cudf_polars/cudf_polars/dsl/ir.py, python/cudf_polars/cudf_polars/dsl/translate.py
Reduced set of version flags; Scan URI validation, Iceberg deletion_files handling, Cache refcount, Sink payload selection, StructFunction dispatch, log/l rewrite, TrueDivide and decimal Multiply rewrite logic simplified or made unconditional.
Decimal and String Operations
python/cudf_polars/cudf_polars/containers/datatype.py, python/cudf_polars/cudf_polars/dsl/expressions/string.py
pl.Decimal precision header now written directly from dtype.precision; polars moved to TYPE_CHECKING for type-only imports; removed older-version validation branches in ZFill, PadStart, and PadEnd.
Predicate Casts and Aggregations
python/cudf_polars/cudf_polars/dsl/ir.py, python/cudf_polars/cudf_polars/dsl/utils/aggregations.py
Removed version guard from predicate-cast stripping; quantile casting and sum post-aggregation null-replacement logic made unconditional.
Test Assertion Helpers
python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py, python/cudf_polars/cudf_polars/testing/asserts.py
Unconditionally use rel_tol/abs_tol keyword names for float-tolerance kwargs in assertions.

Test Suite Compatibility Cleanup

Layer / File(s) Summary
Fixture Parametrization and Select/Explain Tests
python/cudf_polars/tests/dsl/test_serialization.py, python/cudf_polars/tests/experimental/test_explain.py, python/cudf_polars/tests/experimental/test_select.py
StructFunction imported unconditionally; version-gated skip removed from test_serialize_query; fill and decimal dtype checks and warnings simplified and made unconditional.
Expression Tests: Aggregation, Boolean, Casting, and Binary Ops
python/cudf_polars/tests/expressions/*
Removed many version-gated xfail/skip markers and conditional dtype-check toggles; updated xfail fixture usage to xfail_if_sorted.
Ranking, Rolling Window, and Window Function Tests
python/cudf_polars/tests/expressions/*, python/cudf_polars/tests/test_window_functions.py
request fixtures removed from rank/rolling tests; version-based xfail markers eliminated and GPU equivalence assertions simplified.
Sort, String, Struct, and Integration Tests
python/cudf_polars/tests/expressions/test_sort.py, test_stringfunction.py, test_struct.py, and integration tests under python/cudf_polars/tests/
Simplified test_setsorted xfail condition; removed version-dependent parametrization and exception expectations in string tests; removed struct test version-gated xfails; many integration tests switched from version-conditional expectations to unconditional GPU assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • TomAugspurger

  • msarahan

  • bdice

  • pentschev

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.97% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: bumping the minimum Polars version from 1.30 to 1.35 across all configuration and source files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description clearly explains the changes: bumping minimum Polars version to 1.35 and removing version-specific workarounds, which directly aligns with the changeset showing version constraint updates and removal of compatibility code.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
python/cudf_polars/tests/expressions/test_sort.py (1)

62-78: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

test_setsorted no longer checks the sorted-hint behavior.

set_sorted does not change row values, so assert_gpu_result_equal will still pass if we stop propagating sorted metadata. Please keep one assertion on the evaluated column metadata, or cover a downstream operation whose correctness depends on that hint.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/tests/expressions/test_sort.py` around lines 62 - 78, The
test_setsorted currently only asserts data equality via assert_gpu_result_equal
on q (the LazyFrame after calling set_sorted), which won't catch lost sorted
metadata because set_sorted doesn't modify row values; update the test to also
assert that the evaluated LazyFrame or its column metadata retains the sorted
hint. Locate test_setsorted (create ldf, call ldf.set_sorted("a",
descending=descending) producing q) and add an assertion that q (or the
materialized frame after q.collect()/q.evaluate()) has the sorted/descending and
nulls_last metadata on column "a" (or exercise a downstream operation that
relies on the sorted hint, e.g., a merge/join/rolling operation using q that
would behave differently without the hint) so the test fails if sorted metadata
is not propagated.
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py (1)

237-257: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This changes rolling sum semantics for all-null windows.

The new top-level path always applies replace_nulls(col, 0, is_top=is_top), regardless of context. In rolling mode that turns a non-empty all-null window into 0, even though the comment here still calls out the required null result for that case.

As per coding guidelines, python/**/*.{py,pyx}: Logic errors producing wrong results - Verify algorithm correctness and data integrity in operations.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/cudf_polars/dsl/utils/aggregations.py` around lines 237 -
257, The aggregation for agg.name == "sum" incorrectly always applies
replace_nulls(col, 0, is_top=is_top) which changes rolling semantics; update the
logic in the agg.name == "sum" branch to branch on the aggregation context
(rolling vs groupby/top) instead of unconditionally calling replace_nulls: for
non-rolling (groupby/top) keep the current replace_nulls(col, 0, is_top=is_top)
behavior, but for rolling produce an expr.NamedExpr(name, ...) that preserves
null for non-empty all-null windows and only fills zeros for empty windows
(i.e., do not call the top-level replace_nulls for rolling). Locate this change
around the agg.name == "sum" block (symbols: agg.name, col, replace_nulls,
is_top, expr.NamedExpr) and implement a conditional based on the rolling context
to restore correct semantics.
python/cudf_polars/tests/expressions/test_rolling.py (1)

321-336: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Incomplete refactoring: request.applymarker() called without arguments will raise TypeError.

The request.applymarker() call on line 330 requires a marker argument but none is provided. This will cause test collection to fail with a TypeError.

Comparing with the other rank tests (test_rank_over_with_ties, test_rank_over_with_null_values, test_rank_over_with_null_group_keys) which correctly removed both the request fixture and any applymarker calls, this function needs the same treatment.

🐛 Proposed fix: Remove unused `request` fixture and no-op `applymarker` call
 `@pytest.mark.parametrize`("method", ["ordinal", "dense", "min", "max", "average"])
 `@pytest.mark.parametrize`("descending", [False, True])
 `@pytest.mark.parametrize`("order_by", [None, ["g2", pl.col("x2") * 2]])
 def test_rank_over(
     engine: pl.GPUEngine,
-    request,
     df: pl.LazyFrame,
     method: RankMethod,
     *,
     descending: bool,
     order_by: None | list[str | pl.Expr],
 ) -> None:
-    request.applymarker()
     q = df.select(
         pl.col("x")
         .rank(method=method, descending=descending)
         .over("g", order_by=order_by)
     )
     assert_gpu_result_equal(q, engine=engine)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/tests/expressions/test_rolling.py` around lines 321 - 336,
The test function test_rank_over contains an unnecessary request.applymarker()
call and an unused request fixture parameter which will raise TypeError; remove
the request parameter from the test_rank_over signature and delete the
request.applymarker() invocation so the function signature matches other rank
tests (e.g., test_rank_over_with_ties) and no longer depends on
request.applymarker.
🧹 Nitpick comments (2)
python/cudf_polars/tests/test_cache.py (1)

16-17: ⚡ Quick win

Don't blanket-xfail this without strict=True.

A non-strict xfail on the whole test will silently accept XPASS and also hides whether the structural cache-node assertions below are still protecting anything. At minimum make the marker strict; ideally split the legacy hit-count assertions into a separate xfailed test and keep the structural checks active.

Minimal improvement
-@pytest.mark.xfail(reason="python no longer manages cache hits")
+@pytest.mark.xfail(reason="python no longer manages cache hits", strict=True)
 def test_cache(engine: pl.GPUEngine):
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/tests/test_cache.py` around lines 16 - 17, The test marked
with pytest.mark.xfail (test_cache) is currently non-strict and may hide XPASS
and suppress useful structural assertions; update the xfail marker to be strict
(pytest.mark.xfail(reason=..., strict=True)) or, better, split the legacy
cache-hit/count assertions into a separate test decorated with
xfail(strict=True) (e.g., test_cache_hit_counts_xfail) while keeping the
original test_cache to retain the structural cache-node assertions; locate the
pytest marker on test_cache and either add strict=True to the marker or move
only the flaky assertions into a new xfailed test so the remaining assertions
continue to run.
python/cudf_polars/tests/expressions/test_agg.py (1)

214-219: ⚡ Quick win

Add degenerate decimal std/var cases while this path is being ungated.

Now that this runs unconditionally, it still only covers a three-row happy path. Empty, all-null, and single-element decimal inputs are the cases most likely to drift when the implementation casts fixed-point values through float first, so I'd extend this before removing the old guard.

As per coding guidelines, "Missing edge case coverage in tests - Include tests for empty, all-null, single-element, and mixed type cases".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/tests/expressions/test_agg.py` around lines 214 - 219,
Extend the test_decimal_std_var coverage by adding degenerate cases (empty
dataframe, all-null column, single-element column, and a mixed-type edge if
relevant) for the same expressions q = decimal_df.select(std=pl.col("a").std(),
var=pl.col("a").var()) used in test_decimal_std_var; create separate small
decimal_df variants (or parametrize the test) that produce an empty frame, a
frame where column "a" is all None, and a frame with a single decimal value,
then call assert_gpu_result_equal(q, engine=engine) for each to ensure std/var
behavior matches CPU for these edge conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@python/cudf_polars/cudf_polars/containers/datatype.py`:
- Around line 47-51: The TypedDict _DecimalDataTypeHeader must allow nullable
precision: change its precision field from int to int | None in
python/cudf_polars/cudf_polars/typing/__init__.py so it matches Polars'
pl.Decimal(None, scale); then update the consumer that reconstructs the dtype
(the call pl.Decimal(header["precision"], header["scale"]) in datatype handling)
to accept and forward None for precision (i.e., pass header["precision"]
directly or guard so pl.Decimal receives None when present) so no type/value
error occurs when precision is None.

In `@python/cudf_polars/cudf_polars/dsl/ir.py`:
- Around line 1972-1980: The code currently strips ColRef float→float casts
because the if-branch checks only for "both floating" and strips; restrict this
to avoid dropping non-idempotent float casts: in the condition around child and
expr.ColRef (the block using plc.traits.is_floating_point,
plc.traits.is_integral, src.plc_type and dst.plc_type.id()), only allow removing
the cast when it is safe/idempotent — e.g., keep the existing integral-same-id
branch (plc.traits.is_integral && src.plc_type.id() == dst.plc_type.id()), and
remove the broad floating-point branch so float→float casts are not stripped;
alternatively add an explicit idempotence check for decimals/scale+precision
equivalence before stripping. Ensure you update the logic in that if that
references child, src.plc_type and dst.plc_type so float casts remain intact
unless proven idempotent.

In `@python/cudf_polars/cudf_polars/dsl/translate.py`:
- Around line 795-807: The current branch that handles "log" incorrectly asserts
base is expr.NamedExpr which is wrong because translator.translate_expr()
returns expr.Expr; instead check if base is an expr.Literal and build the
expr.BinOp(LOG_BASE, child, expr.Literal(...)) in that case, otherwise raise a
targeted NotImplementedError indicating non-literal log bases are not supported;
replace the assert isinstance(base, expr.NamedExpr) with an if/else that uses
isinstance(base, expr.Literal) and raises NotImplementedError for other expr
types (keep the returned expr.BinOp construction using
plc.binaryop.BinaryOperator.LOG_BASE, child, and base.value).

In `@python/cudf_polars/tests/expressions/test_stringfunction.py`:
- Around line 590-595: The test currently expects Polars to raise
pl.exceptions.InvalidOperationError for negative zfill widths but leaves
cudf_except empty, allowing GPU to succeed and hide the mismatch; update the
assertion so both CPU and GPU paths must exhibit the same failure: set
cudf_except to include the same exception type (or otherwise require that
assert_collect_raises(q, polars_except=pl.exceptions.InvalidOperationError,
cudf_except=(pl.exceptions.InvalidOperationError,)) or the equivalent GPU-side
exception) so str.zfill(pl.col("fill")) fails consistently across CPU and GPU
rather than permitting a divergent success.

---

Outside diff comments:
In `@python/cudf_polars/cudf_polars/dsl/utils/aggregations.py`:
- Around line 237-257: The aggregation for agg.name == "sum" incorrectly always
applies replace_nulls(col, 0, is_top=is_top) which changes rolling semantics;
update the logic in the agg.name == "sum" branch to branch on the aggregation
context (rolling vs groupby/top) instead of unconditionally calling
replace_nulls: for non-rolling (groupby/top) keep the current replace_nulls(col,
0, is_top=is_top) behavior, but for rolling produce an expr.NamedExpr(name, ...)
that preserves null for non-empty all-null windows and only fills zeros for
empty windows (i.e., do not call the top-level replace_nulls for rolling).
Locate this change around the agg.name == "sum" block (symbols: agg.name, col,
replace_nulls, is_top, expr.NamedExpr) and implement a conditional based on the
rolling context to restore correct semantics.

In `@python/cudf_polars/tests/expressions/test_rolling.py`:
- Around line 321-336: The test function test_rank_over contains an unnecessary
request.applymarker() call and an unused request fixture parameter which will
raise TypeError; remove the request parameter from the test_rank_over signature
and delete the request.applymarker() invocation so the function signature
matches other rank tests (e.g., test_rank_over_with_ties) and no longer depends
on request.applymarker.

In `@python/cudf_polars/tests/expressions/test_sort.py`:
- Around line 62-78: The test_setsorted currently only asserts data equality via
assert_gpu_result_equal on q (the LazyFrame after calling set_sorted), which
won't catch lost sorted metadata because set_sorted doesn't modify row values;
update the test to also assert that the evaluated LazyFrame or its column
metadata retains the sorted hint. Locate test_setsorted (create ldf, call
ldf.set_sorted("a", descending=descending) producing q) and add an assertion
that q (or the materialized frame after q.collect()/q.evaluate()) has the
sorted/descending and nulls_last metadata on column "a" (or exercise a
downstream operation that relies on the sorted hint, e.g., a merge/join/rolling
operation using q that would behave differently without the hint) so the test
fails if sorted metadata is not propagated.

---

Nitpick comments:
In `@python/cudf_polars/tests/expressions/test_agg.py`:
- Around line 214-219: Extend the test_decimal_std_var coverage by adding
degenerate cases (empty dataframe, all-null column, single-element column, and a
mixed-type edge if relevant) for the same expressions q =
decimal_df.select(std=pl.col("a").std(), var=pl.col("a").var()) used in
test_decimal_std_var; create separate small decimal_df variants (or parametrize
the test) that produce an empty frame, a frame where column "a" is all None, and
a frame with a single decimal value, then call assert_gpu_result_equal(q,
engine=engine) for each to ensure std/var behavior matches CPU for these edge
conditions.

In `@python/cudf_polars/tests/test_cache.py`:
- Around line 16-17: The test marked with pytest.mark.xfail (test_cache) is
currently non-strict and may hide XPASS and suppress useful structural
assertions; update the xfail marker to be strict (pytest.mark.xfail(reason=...,
strict=True)) or, better, split the legacy cache-hit/count assertions into a
separate test decorated with xfail(strict=True) (e.g.,
test_cache_hit_counts_xfail) while keeping the original test_cache to retain the
structural cache-node assertions; locate the pytest marker on test_cache and
either add strict=True to the marker or move only the flaky assertions into a
new xfailed test so the remaining assertions continue to run.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7802ed01-351f-45b8-84dc-9d0e056c1d0e

📥 Commits

Reviewing files that changed from the base of the PR and between b71adc2 and 7defd7c.

📒 Files selected for processing (36)
  • .pre-commit-config.yaml
  • conda/environments/all_cuda-129_arch-aarch64.yaml
  • conda/environments/all_cuda-129_arch-x86_64.yaml
  • conda/environments/all_cuda-131_arch-aarch64.yaml
  • conda/environments/all_cuda-131_arch-x86_64.yaml
  • conda/recipes/cudf-polars/recipe.yaml
  • dependencies.yaml
  • python/cudf_polars/cudf_polars/containers/datatype.py
  • python/cudf_polars/cudf_polars/dsl/expressions/string.py
  • python/cudf_polars/cudf_polars/dsl/ir.py
  • python/cudf_polars/cudf_polars/dsl/translate.py
  • python/cudf_polars/cudf_polars/dsl/utils/aggregations.py
  • python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py
  • python/cudf_polars/cudf_polars/testing/asserts.py
  • python/cudf_polars/cudf_polars/utils/versions.py
  • python/cudf_polars/pyproject.toml
  • python/cudf_polars/tests/dsl/test_serialization.py
  • python/cudf_polars/tests/experimental/test_explain.py
  • python/cudf_polars/tests/experimental/test_select.py
  • python/cudf_polars/tests/expressions/test_agg.py
  • python/cudf_polars/tests/expressions/test_booleanfunction.py
  • python/cudf_polars/tests/expressions/test_casting.py
  • python/cudf_polars/tests/expressions/test_numeric_binops.py
  • python/cudf_polars/tests/expressions/test_numeric_unaryops.py
  • python/cudf_polars/tests/expressions/test_rolling.py
  • python/cudf_polars/tests/expressions/test_sort.py
  • python/cudf_polars/tests/expressions/test_stringfunction.py
  • python/cudf_polars/tests/expressions/test_struct.py
  • python/cudf_polars/tests/test_cache.py
  • python/cudf_polars/tests/test_drop_nulls.py
  • python/cudf_polars/tests/test_groupby.py
  • python/cudf_polars/tests/test_join.py
  • python/cudf_polars/tests/test_mapfunction.py
  • python/cudf_polars/tests/test_scan.py
  • python/cudf_polars/tests/test_select.py
  • python/cudf_polars/tests/test_window_functions.py
💤 Files with no reviewable changes (2)
  • python/cudf_polars/tests/expressions/test_numeric_unaryops.py
  • python/cudf_polars/tests/experimental/test_explain.py

Comment thread python/cudf_polars/cudf_polars/containers/datatype.py
Comment thread python/cudf_polars/cudf_polars/dsl/ir.py
Comment thread python/cudf_polars/cudf_polars/dsl/translate.py Outdated
Comment thread python/cudf_polars/tests/expressions/test_stringfunction.py
@GPUtester GPUtester moved this to In Progress in cuDF Python May 11, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
python/cudf_polars/cudf_polars/dsl/translate.py (1)

800-807: ⚡ Quick win

Prefer explicit NotImplementedError over assert for feature detection.

While the type check has been corrected to expr.Literal, using assert for runtime feature detection is non-idiomatic. Assertions can be disabled with -O and convey "invariant violation" rather than "unsupported input". An explicit check provides a clearer error message and correct semantics.

Proposed fix
         (child, base) = children
-        assert isinstance(base, expr.Literal)
+        if not isinstance(base, expr.Literal):
+            raise NotImplementedError("log with non-literal base is not supported")
         return expr.BinOp(
             dtype,
             plc.binaryop.BinaryOperator.LOG_BASE,
             child,
             expr.Literal(dtype, base.value),
         )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/cudf_polars/dsl/translate.py` around lines 800 - 807,
Replace the runtime assertion with an explicit check and raise
NotImplementedError when the second child is not a literal: instead of using
"assert isinstance(base, expr.Literal)" in the block that returns
expr.BinOp(..., plc.binaryop.BinaryOperator.LOG_BASE, ...), test
"isinstance(base, expr.Literal)" and raise NotImplementedError with a clear
message (e.g., "LOG_BASE requires a literal base") referencing the symbol names
expr.Literal and plc.binaryop.BinaryOperator.LOG_BASE so callers see a proper
runtime error rather than an assert that can be disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@python/cudf_polars/cudf_polars/dsl/translate.py`:
- Around line 800-807: Replace the runtime assertion with an explicit check and
raise NotImplementedError when the second child is not a literal: instead of
using "assert isinstance(base, expr.Literal)" in the block that returns
expr.BinOp(..., plc.binaryop.BinaryOperator.LOG_BASE, ...), test
"isinstance(base, expr.Literal)" and raise NotImplementedError with a clear
message (e.g., "LOG_BASE requires a literal base") referencing the symbol names
expr.Literal and plc.binaryop.BinaryOperator.LOG_BASE so callers see a proper
runtime error rather than an assert that can be disabled.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 522a6f3f-d638-4299-8b35-73b4d7801a1a

📥 Commits

Reviewing files that changed from the base of the PR and between 7defd7c and 3ca8964.

📒 Files selected for processing (7)
  • conda/environments/all_cuda-129_arch-aarch64.yaml
  • conda/environments/all_cuda-129_arch-x86_64.yaml
  • conda/environments/all_cuda-131_arch-aarch64.yaml
  • conda/environments/all_cuda-131_arch-x86_64.yaml
  • dependencies.yaml
  • python/cudf_polars/cudf_polars/dsl/translate.py
  • python/cudf_polars/tests/expressions/test_rolling.py
✅ Files skipped from review due to trivial changes (3)
  • conda/environments/all_cuda-129_arch-x86_64.yaml
  • conda/environments/all_cuda-131_arch-aarch64.yaml
  • conda/environments/all_cuda-131_arch-x86_64.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • dependencies.yaml
  • conda/environments/all_cuda-129_arch-aarch64.yaml

@mroeschke
mroeschke requested a review from a team as a code owner May 11, 2026 21:45

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ci/test_wheel_cudf_polars.sh (1)

74-83: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle timeout expiry explicitly so CI failures are diagnosable.

With the timeout set to 15m, timeouts become a realistic failure mode; those exits are currently reported as generic test failures. The GNU timeout command returns exit code 124 when the time limit is reached. Please branch on this exit code to log a timeout-specific message, improving CI diagnostics.

Suggested patch
-    timeout 15m ./ci/run_cudf_polars_pytests.sh \
+    timeout 15m ./ci/run_cudf_polars_pytests.sh \
         "${COVERAGE_ARGS[@]}" \
         --numprocesses=8 \
         --dist=worksteal \
         --junitxml="${RAPIDS_TESTS_DIR}/junit-cudf-polars-${version}.xml"
 
-    if [ $? -ne 0 ]; then
+    test_exit=$?
+    if [ ${test_exit} -eq 124 ]; then
+        EXITCODE=1
+        FAILED+=("${version}")
+        rapids-logger "Tests timed out after 15m for polars==${version}"
+    elif [ ${test_exit} -ne 0 ]; then
         EXITCODE=1
         FAILED+=("${version}")
         rapids-logger "Tests failed for polars==${version}"
     else
         PASSED+=("${version}")

Per the coding guideline: "Check for proper error handling and meaningful error messages."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ci/test_wheel_cudf_polars.sh` around lines 74 - 83, The timeout invocation of
./ci/run_cudf_polars_pytests.sh needs explicit handling for GNU timeout's 124
exit so CI can report timeouts separately: after the timeout command (the block
using timeout 15m ./ci/run_cudf_polars_pytests.sh) inspect the exit status ($?)
and if it equals 124 call rapids-logger with a timeout-specific message (e.g.,
"Tests timed out for polars==${version}"), set EXITCODE=1 and add "${version}"
to FAILED—otherwise retain the existing failure branch that logs generic test
failures; reference the existing EXITCODE, FAILED, and rapids-logger symbols
when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@ci/test_wheel_cudf_polars.sh`:
- Around line 74-83: The timeout invocation of ./ci/run_cudf_polars_pytests.sh
needs explicit handling for GNU timeout's 124 exit so CI can report timeouts
separately: after the timeout command (the block using timeout 15m
./ci/run_cudf_polars_pytests.sh) inspect the exit status ($?) and if it equals
124 call rapids-logger with a timeout-specific message (e.g., "Tests timed out
for polars==${version}"), set EXITCODE=1 and add "${version}" to
FAILED—otherwise retain the existing failure branch that logs generic test
failures; reference the existing EXITCODE, FAILED, and rapids-logger symbols
when making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ccffae54-d904-4eb3-a7c5-50238813513e

📥 Commits

Reviewing files that changed from the base of the PR and between 3ca8964 and e59bc97.

📒 Files selected for processing (1)
  • ci/test_wheel_cudf_polars.sh

@mroeschke

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit a1ec1e6 into NVIDIA:main May 12, 2026
235 of 238 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python May 12, 2026
@mroeschke
mroeschke deleted the feat/cudf_polars/min_bump branch May 12, 2026 00:38
shrshi pushed a commit to shrshi/cudf that referenced this pull request May 12, 2026
Broken off from NVIDIA#22048

Updates the minimum Polars version in cudf_polars based on the minimum supported Polars version in cloud environments NVIDIA#22048 (comment)

Code changes are purely removals (with minor reorganizations). Commits are split by workarounds removed per version.

Additionally, shortens the `timeout` of each test run per Polars version to 15 minutes each from 1 hour. A "normal" test run per Polars version in CI should complete in 2-3 minutes so 15 minutes should hopefully be OK

Authors:
  - Matthew Roeschke (https://github.com/mroeschke)

Approvers:
  - Tom Augspurger (https://github.com/TomAugspurger)
  - Matthew Murray (https://github.com/Matt711)
  - Bradley Dice (https://github.com/bdice)

URL: NVIDIA#22459
@coderabbitai coderabbitai Bot mentioned this pull request May 15, 2026
3 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Jun 30, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change cudf-polars Issues specific to cudf-polars improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants