Fix GroupBy.apply result assembly, UDF closure side effects, and empty-frame dtypes - #23272
Merged
rapids-bot[bot] merged 8 commits intoJul 18, 2026
Merged
Conversation
github-actions
Bot
added
Python
galipremsagar
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesPandas compatibility behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
python/cudf/cudf/pandas/fast_slow_proxy.py (1)
1398-1414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both identity-preserving branches.
The supplied downstream test covers object-dtype ndarray identity, but not the new list/dict behavior. Add tests for empty and single-element containers, nested unchanged containers, proxy-containing containers that must rebuild, key/value transformations, and supported container subclasses. Add the required unit benchmark for this bug-fix contribution.
As per coding guidelines, bug-fix contributions require unit tests and unit benchmarks, with Python changes validated through the repository’s pre-commit checks.
Also applies to: 1486-1498
🤖 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/cudf/pandas/fast_slow_proxy.py` around lines 1398 - 1414, Add regression tests for the container transformation logic in _transform_arg, covering empty and single-element lists/dicts, nested unchanged containers, proxy-containing containers that rebuild, key and value transformations, and supported container subclasses. Add the required unit benchmark for this bug fix, and run the repository’s Python pre-commit checks to validate the changes.Source: Coding guidelines
🤖 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/cudf/core/groupby/groupby.py`:
- Around line 2418-2422: Update the include_groups parameter documentation in
the groupby apply docstring to remove the outdated statement that True attempts
to apply func to groupings. Ensure the docstring consistently states that only
False is accepted and True raises ValueError, matching the validation in the
groupby apply implementation.
- Around line 2324-2335: Update the transform-like UDF branch in the groupby
result assembly to validate each group’s output size against the corresponding
expected size using the existing offsets, rather than comparing only the
aggregate chunk length to len(self.obj). Only construct and assign the
MultiIndex when all per-group sizes match, preserving correct row alignment for
unevenly shrinking or expanding groups.
In `@python/cudf/cudf/pandas/fast_slow_proxy.py`:
- Around line 1398-1414: Update the transformed iterable construction in
_transform_arg so list subclasses receive the transformed values through the
original generator-style constructor input rather than a concrete list. Preserve
the unchanged-object identity return and ensure subclass-specific iterable
construction behavior remains intact.
---
Nitpick comments:
In `@python/cudf/cudf/pandas/fast_slow_proxy.py`:
- Around line 1398-1414: Add regression tests for the container transformation
logic in _transform_arg, covering empty and single-element lists/dicts, nested
unchanged containers, proxy-containing containers that rebuild, key and value
transformations, and supported container subclasses. Add the required unit
benchmark for this bug fix, and run the repository’s Python pre-commit checks to
validate the changes.
🪄 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: 72ad9afb-4a31-4e75-a223-dad629c342ec
📒 Files selected for processing (9)
python/cudf/cudf/core/column/column.pypython/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/core/indexed_frame.pypython/cudf/cudf/core/series.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/dataframe/indexing/test_setitem.pypython/cudf/cudf/tests/groupby/test_apply.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/tests/groupby/test_apply.py
Contributor
Author
|
Merged in upstream/main and resolved the two conflicts:
Re-validated after the merge + review changes: pandas-tests |
Contributor
Author
|
/okay to test 731dc29 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf/cudf/pandas/fast_slow_proxy.py (1)
1505-1508: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare mapping entries without invoking overridden mapping methods.
zip(transformed_dict, arg)invokesarg.__iter__, andarg[old_k]invokesarg.__getitem__. An unchangeddictsubclass overriding either can now fail during identity detection even though the precedingarg.items()transformation succeeded. Compare.items()pairs directly and add a regression test.Proposed fix
if len(transformed_dict) == len(arg) and all( - new_k is old_k and transformed_dict[new_k] is arg[old_k] - for new_k, old_k in zip(transformed_dict, arg, strict=True) + new_k is old_k and new_v is old_v + for (new_k, new_v), (old_k, old_v) in zip( + transformed_dict.items(), arg.items(), strict=True + ) ):🤖 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/cudf/pandas/fast_slow_proxy.py` around lines 1505 - 1508, Update the identity check in the surrounding transformation logic to compare the transformed mapping’s items directly with the original entries, avoiding iteration and item access through overridden mapping methods on arg. Preserve the existing key and value identity checks, and add a regression test covering an unchanged dict subclass that overrides __iter__ or __getitem__.python/cudf/cudf/core/dataframe.py (1)
6577-6603: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
columnsin themethod="table"path. This branch still passes everydata_df._columnsto libcudf and returns every column, whilecolumnsis only applied in the per-column branch.df.quantile(columns=["a"], method="table")should restrict the computation to the requested subset; add a regression test.🤖 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/cudf/core/dataframe.py` around lines 6577 - 6603, Update the method="table" branch to compute and return only the columns specified by columns, using the selected column names and corresponding data_df._columns when building the libcudf table and result mapping; preserve all-column behavior when columns is unspecified. Add a regression test confirming df.quantile(columns=["a"], method="table") excludes other columns.Source: Coding guidelines
🧹 Nitpick comments (2)
python/cudf/cudf/tests/groupby/test_apply.py (1)
766-767: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin
include_groups=Falsein both new regression cases.These tests should exercise the same explicit pandas-compatibility contract as the surrounding test. Relying on each library’s default can make the reference and actual calls diverge across supported pandas versions; pass
include_groups=Falseto both sides.Proposed change
- expected = pdf.groupby("k").apply(make_swap_sizes(pd.Series)) - actual = gdf.groupby("k").apply(make_swap_sizes(cudf.Series)) + expected = pdf.groupby("k").apply( + make_swap_sizes(pd.Series), include_groups=False + ) + actual = gdf.groupby("k").apply( + make_swap_sizes(cudf.Series), include_groups=False + ) - expected = pdf.groupby("k").apply(make_fresh_index(pd.Series)) - actual = gdf.groupby("k").apply(make_fresh_index(cudf.Series)) + expected = pdf.groupby("k").apply( + make_fresh_index(pd.Series), include_groups=False + ) + actual = gdf.groupby("k").apply( + make_fresh_index(cudf.Series), include_groups=False + )Also applies to: 784-785
🤖 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/cudf/tests/groupby/test_apply.py` around lines 766 - 767, Update both new regression cases around the groupby apply calls, including the lines using expected and actual, to pass include_groups=False explicitly to pandas and cuDF GroupBy.apply. Keep the callback and other arguments unchanged, and apply the same explicit option to both reference and actual calls.python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py (1)
694-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the list-subclass constructor contract.
This only checks output type; it would still pass if Line 1417 passed
transformed_listinstead of an iterator. Record the constructor argument type and assert the rebuilt subclass received a non-list iterable.Proposed test hardening
class MyList(list): - pass + def __init__(self, values=()): + self.received_materialized_list = isinstance(values, list) + super().__init__(values) my_list = MyList([1, x]) result = transform(my_list) assert result is not my_list assert type(result) is MyList + assert not result.received_materialized_list assert type(result[1]) is expected_typeAs per coding guidelines, “Add unit tests and unit benchmarks for feature and bug-fix contributions.”
🤖 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/cudf_pandas_tests/test_fast_slow_proxy.py` around lines 694 - 702, Harden the MyList case in the transform test by recording the argument received by its constructor and asserting it is a non-list iterable. Update the subclass used near transform(my_list) to capture construction input while preserving the existing assertions for a distinct MyList result and transformed element type.Source: Coding guidelines
🤖 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/cudf/pandas/fast_slow_proxy.py`:
- Around line 1414-1417: Add a unit benchmark covering the container
transformation path around the recursive proxy logic that returns
type(arg)(iter(transformed_list)). Benchmark both unchanged containers and
containers containing proxy values, measuring transformation performance and
guarding against regressions from materialization and identity scanning.
---
Outside diff comments:
In `@python/cudf/cudf/core/dataframe.py`:
- Around line 6577-6603: Update the method="table" branch to compute and return
only the columns specified by columns, using the selected column names and
corresponding data_df._columns when building the libcudf table and result
mapping; preserve all-column behavior when columns is unspecified. Add a
regression test confirming df.quantile(columns=["a"], method="table") excludes
other columns.
In `@python/cudf/cudf/pandas/fast_slow_proxy.py`:
- Around line 1505-1508: Update the identity check in the surrounding
transformation logic to compare the transformed mapping’s items directly with
the original entries, avoiding iteration and item access through overridden
mapping methods on arg. Preserve the existing key and value identity checks, and
add a regression test covering an unchanged dict subclass that overrides
__iter__ or __getitem__.
---
Nitpick comments:
In `@python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py`:
- Around line 694-702: Harden the MyList case in the transform test by recording
the argument received by its constructor and asserting it is a non-list
iterable. Update the subclass used near transform(my_list) to capture
construction input while preserving the existing assertions for a distinct
MyList result and transformed element type.
In `@python/cudf/cudf/tests/groupby/test_apply.py`:
- Around line 766-767: Update both new regression cases around the groupby apply
calls, including the lines using expected and actual, to pass
include_groups=False explicitly to pandas and cuDF GroupBy.apply. Keep the
callback and other arguments unchanged, and apply the same explicit option to
both reference and actual calls.
🪄 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: 69d66ed9-894c-41ea-b20c-0ae3fb24e03c
📒 Files selected for processing (8)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/core/series.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/dataframe/test_constructors.pypython/cudf/cudf/tests/groupby/test_apply.pypython/cudf/cudf_pandas_tests/test_fast_slow_proxy.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cudf/cudf/core/series.py
- python/cudf/cudf/core/groupby/groupby.py
mroeschke
reviewed
Jul 15, 2026
Contributor
Author
|
/okay to test 804b3f9 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf/cudf/core/groupby/groupby.py (2)
2304-2346: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle mixed
None/Seriesoutputs explicitly. This branch assumes every chunk is aSeries; if a later group returnsNone,.indexwill fail, and if the first result isNonethe mixed results fall through the generic concat path. Filter outNoneresults before dispatching and add a regression test.🤖 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/cudf/core/groupby/groupby.py` around lines 2304 - 2346, Update the groupby result dispatch before the Series-index comparison to handle mixed None/Series outputs explicitly: remove or separately track None chunks, use the non-None Series results for index-based branching, and preserve the appropriate None behavior when constructing the final result. Ensure the path does not access `.index` on None or send a first-None mixed result through generic concatenation, and add a regression test covering both result orderings.Source: Coding guidelines
2333-2346: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve all returned index levels. This branch collapses
result.indextoresult.index._column, so a UDF that returns aMultiIndexloses the extra levels and their names. Rebuild the inner index from every returned level instead.🤖 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/cudf/core/groupby/groupby.py` around lines 2333 - 2346, The index reconstruction in the groupby UDF result branch must preserve every level of a returned MultiIndex. Update the logic around result.index and MultiIndex._from_data to gather and include each returned index level and its corresponding name, rather than using only result.index._column, while retaining the grouping levels and row alignment.
🤖 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 `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 2304-2346: Update the groupby result dispatch before the
Series-index comparison to handle mixed None/Series outputs explicitly: remove
or separately track None chunks, use the non-None Series results for index-based
branching, and preserve the appropriate None behavior when constructing the
final result. Ensure the path does not access `.index` on None or send a
first-None mixed result through generic concatenation, and add a regression test
covering both result orderings.
- Around line 2333-2346: The index reconstruction in the groupby UDF result
branch must preserve every level of a returned MultiIndex. Update the logic
around result.index and MultiIndex._from_data to gather and include each
returned index level and its corresponding name, rather than using only
result.index._column, while retaining the grouping levels and row alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5add4463-9ec6-4507-b509-a0f534b9e2c7
📒 Files selected for processing (7)
python/cudf/benchmarks/internal/bench_fast_slow_proxy.pypython/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/tests/dataframe/methods/test_reductions.pypython/cudf/cudf/tests/groupby/test_apply.pypython/cudf/cudf_pandas_tests/test_fast_slow_proxy.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cudf/cudf/pandas/fast_slow_proxy.py
- python/cudf/cudf/tests/groupby/test_apply.py
mroeschke
approved these changes
Jul 15, 2026
galipremsagar
added
the
5 - Ready to Merge
Contributor
Author
|
/okay to test 0c3de7a |
…ame dtypes
Fixes 26 of the 28 failing tests in pandas' tests/groupby/test_apply.py
under cudf.pandas (137 tests: 26 fixed, 2 inherent) and removes the
fixed plugin entries; the two kept entries document why they cannot be
fixed (datetime.date type identity on the GPU round trip; CoW block
identity across independent fallback conversions).
cudf.pandas proxy:
* _transform_arg now preserves the identity of lists and dicts whose
elements needed no conversion (as the object-ndarray branch already
did). A user function may close over a mutable container and mutate
it for its side effects (e.g. names.append(group.name) inside
groupby.apply); copying the container silently discarded those side
effects on both the fast attempt and the pandas fallback.
GroupBy.apply result assembly (pandas parity, verified empirically):
* All-None DataFrameGroupBy results return an empty frame keeping the
value columns and dtypes (pandas GH9684/GH57775).
* Series results sharing an identical index stack into one row per
group with columns given by the common index, propagating a
consistent Series name to the columns axis (GH6124); Series results
with differing indexes concatenate lengthwise under the group keys
(GH8467). This replaces the row-count heuristics that mislabeled
columns and mis-shaped results.
* Transform results (chunks indexed like their input) restore the
original row order regardless of sort, like pandas'
_concat_objects; the final sort_index is removed since group-keyed
results are already emitted in sorted key order and pandas preserves
the UDF's within-group row order (GH52444).
* include_groups=True raises ValueError, matching pandas 3.0.
Other:
* DataFrame({'a': []}) defaults untyped empty sequences to float64
like pandas' constructor (numpy's empty-array default), unlike
Series([]) which stays object.
* reset_index derives the result columns dtype via pandas'
Index.insert provenance instead of re-inferring from merged labels,
and Series.reset_index resolves the value-column name before
resetting (pandas' to_frame(name).reset_index() semantics).
* as_column routes stdlib datetime/timedelta elements through the
pandas object path so mixed datetime+non-datetime lists raise
MixedTypeError instead of silently coercing.
* Remove a stale workaround in test_groupby_apply_return_col_from_df
and a stale conditional xfail in test_dataframe_assign_scalar, both
now matching pandas exactly.
…ntainer identity, docs - GroupBy.apply: drop the aggregate-length "transform-like" branch. Its total-row-count check could pass coincidentally (uneven shrink/expand, or per-group-length matches with a rewritten index) and then stamp the wrong rows with grouped_values' index. The GH8467 concat-with-keys branch already implements pandas' _concat_objects(not_indexed_same=True) exactly (keys repeated per actual chunk length, UDF-returned index kept as the inner level) and produces identical results for true transforms, so the heuristic branch is removed rather than re-gated on offsets. - Remove the stale include_groups=True sentence from the apply docstring. - fast_slow_proxy._transform_arg: rebuilt list subclasses receive an iterator constructor argument again (not a materialized list), and unchanged plain tuples now preserve identity so containers enclosing them keep theirs (a closed-over dict holding a tuple was still copied). - Tests: _transform_arg identity preservation and rebuild propagation (lists/dicts/tuples/list subclasses), GroupBy.apply misaligned and fresh-index Series results vs pandas, and DataFrame constructor dtypes for empty iterator (float64) and empty range (int64, matching pandas' np.arange conversion - the merge keeps Iterator and drops range from the float64 coercion accordingly).
… columns in table path, proxy benchmarks - Use names.pop() for the single-name columns-axis rename (review suggestion). - Compare dict entries via items() in _transform_arg's unchanged-identity scan so mapping subclasses overriding __iter__/__getitem__ still work; regression test included. - DataFrame.quantile now honors the cudf-specific columns argument in the method="table" path by filtering the frame up front (previously only the per-column path filtered); regression test covers both methods. - Pin include_groups=False explicitly in the two new groupby.apply regression tests. - Add benchmarks for _transform_arg container transformation (unchanged identity path and proxy-rebuild path for lists and dicts).
galipremsagar
force-pushed
the
groupby-apply-fixes
branch
from
July 17, 2026 01:37
0c3de7a to
5f92031
Compare
Contributor
Author
|
/okay to test 5f92031 |
Contributor
Author
|
/okay to test 27c4949 |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 17, 2026
The latest-deps conda CI jobs started failing on every PR (e.g. [this run on #23272](https://github.com/rapidsai/cudf/actions/runs/29585048860)) with pyarrow 25.0.0 in the environment — feather tests/doctests (`pyarrow.feather` deprecated as of 24), pylibcudf quantiles (`SortOptions(null_placement=)` deprecated in 25), ORC tests (out-of-ns-range timestamps now raise `ArrowInvalid` instead of silently overflowing), and the narwhals suite. cudf pins `pyarrow>=19.0.0,<24` (#22229) in `dependencies.yaml` for the conda, requirements, and pyproject outputs — but the hand-maintained conda recipes only declare `pyarrow>=19.0.0` with **no upper bound** (`conda/recipes/cudf/recipe.yaml` run dependency and `conda/recipes/pylibcudf/recipe.yaml` run constraint). The conda test environments don't list pyarrow directly (only the oldest-deps matrix pins `pyarrow==19.*`), so the env solve takes the bound from the built packages, and with the recipes unbounded the solver picked pyarrow 25.0.0. This is also how the narwhals job got pyarrow 25: its env installs the built cudf conda package in the same solve. This mirrors the `dependencies.yaml` bound into both recipes, which constrains every conda test environment that installs the built packages. Note: the wheel jobs were unaffected because the pip/pyproject metadata carries the `<24` bound. The remaining failure in the linked run (`conda-python-other-tests`) was a runner infra flake (`nvidia-smi`: "No devices were found") — retry only. For whenever the pin is actually lifted (#22229): the test-suite adaptations needed for pyarrow 24/25 (feather→`pyarrow.ipc` migration, per-sort-key `null_placement`, ORC timestamp-range handling, narwhals deselects) were worked out and verified in [9a3a288](galipremsagar@9a3a288019) (previous head of this branch). Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #23319
Contributor
Author
|
/okay to test af4beb8 |
…ply-fixes # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test b85de3c |
Contributor
Author
|
/merge |
rapids-bot
Bot
merged commit Jul 18, 2026
ae95c7b
into
NVIDIA:release/26.08
233 of 239 checks passed
davidwendt
pushed a commit
to wjxiz1992/cudf
that referenced
this pull request
Jul 21, 2026
The latest-deps conda CI jobs started failing on every PR (e.g. [this run on NVIDIA#23272](https://github.com/rapidsai/cudf/actions/runs/29585048860)) with pyarrow 25.0.0 in the environment — feather tests/doctests (`pyarrow.feather` deprecated as of 24), pylibcudf quantiles (`SortOptions(null_placement=)` deprecated in 25), ORC tests (out-of-ns-range timestamps now raise `ArrowInvalid` instead of silently overflowing), and the narwhals suite. cudf pins `pyarrow>=19.0.0,<24` (NVIDIA#22229) in `dependencies.yaml` for the conda, requirements, and pyproject outputs — but the hand-maintained conda recipes only declare `pyarrow>=19.0.0` with **no upper bound** (`conda/recipes/cudf/recipe.yaml` run dependency and `conda/recipes/pylibcudf/recipe.yaml` run constraint). The conda test environments don't list pyarrow directly (only the oldest-deps matrix pins `pyarrow==19.*`), so the env solve takes the bound from the built packages, and with the recipes unbounded the solver picked pyarrow 25.0.0. This is also how the narwhals job got pyarrow 25: its env installs the built cudf conda package in the same solve. This mirrors the `dependencies.yaml` bound into both recipes, which constrains every conda test environment that installs the built packages. Note: the wheel jobs were unaffected because the pip/pyproject metadata carries the `<24` bound. The remaining failure in the linked run (`conda-python-other-tests`) was a runner infra flake (`nvidia-smi`: "No devices were found") — retry only. For whenever the pin is actually lifted (NVIDIA#22229): the test-suite adaptations needed for pyarrow 24/25 (feather→`pyarrow.ipc` migration, per-sort-key `null_placement`, ORC timestamp-range handling, narwhals deselects) were worked out and verified in [9a3a288](galipremsagar@9a3a288019) (previous head of this branch). Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23319
davidwendt
pushed a commit
to wjxiz1992/cudf
that referenced
this pull request
Jul 21, 2026
…y-frame dtypes (NVIDIA#23272) Running pandas' own test suite under `cudf.pandas`, `tests/groupby/test_apply.py` had 28 failing tests. This PR fixes 26 of them (137 tests: 135 pass; the 2 remaining are inherent and keep documented plugin entries) and removes the fixed xfail entries from the pandas-testing plugin. ### cudf.pandas: UDF closure side effects were silently discarded `_transform_arg` rebuilt lists and dicts even when no element needed proxy conversion. The rebuilt container fails `_replace_closurevars`' identity check, so user functions were rebuilt around a *copy* of their closed-over mutable containers — a UDF like `lambda g: names.append(g.name)` appended into a throwaway copy on both the fast attempt and the pandas fallback, and the user's list stayed empty. The list/dict branches are now identity-preserving when unchanged, mirroring the existing object-ndarray branch. ### `GroupBy.apply` result assembly (pandas parity, all verified empirically on 3.0.3) - All-None `DataFrameGroupBy` results return an empty frame keeping the value columns and dtypes (pandas GH9684/GH57775). - Series results sharing an identical index stack into one row per group with columns given by the common index; a consistent Series name becomes the columns-axis name (GH6124). Series results with differing indexes concatenate lengthwise under the group keys (GH8467). This replaces row-count heuristics that mislabeled columns and mis-shaped results. - Transform results (chunks indexed like their input) restore the original row order regardless of `sort`, like pandas' `_concat_objects`; the final `sort_index` is removed since group-keyed results are already emitted in sorted key order and pandas preserves the UDF's within-group row order (GH52444). - `include_groups=True` raises `ValueError`, matching pandas 3.0. ### Supporting fixes - `DataFrame({"a": []})` defaults untyped empty sequences to float64 like pandas' constructor (numpy's empty-array default); `Series([])` stays object. - `reset_index` derives the result columns dtype via pandas' `Index.insert` provenance instead of re-inferring from the merged labels, and `Series.reset_index` resolves the value-column name before resetting (pandas' `to_frame(name).reset_index()` semantics). - `as_column` routes stdlib `datetime`/`timedelta` elements through the pandas object path, so mixed datetime+non-datetime lists raise `MixedTypeError` instead of silently coercing. - Removed a stale workaround in `test_groupby_apply_return_col_from_df` and a stale conditional xfail in `test_dataframe_assign_scalar` — both now match pandas exactly. ### Validation - pandas-tests `tests/groupby/test_apply.py`: 28 failed → 135/137 pass (2 inherent, documented). - pandas-tests reset_index/constructors/groupby neighbors: only pre-existing known failures. - cuDF classic: groupby suites clean including under `NO_EXTERNAL_ONLY_APIS=1`; dataframe/series/reshape/indexes/input_output sweep (70k+ tests) green. - `cudf.pandas` proxy unit tests pass. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: NVIDIA#23272
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Running pandas' own test suite under
cudf.pandas,tests/groupby/test_apply.pyhad 28 failing tests. This PR fixes 26 of them (137 tests: 135 pass; the 2 remaining are inherent and keep documented plugin entries) and removes the fixed xfail entries from the pandas-testing plugin.cudf.pandas: UDF closure side effects were silently discarded
_transform_argrebuilt lists and dicts even when no element needed proxy conversion. The rebuilt container fails_replace_closurevars' identity check, so user functions were rebuilt around a copy of their closed-over mutable containers — a UDF likelambda g: names.append(g.name)appended into a throwaway copy on both the fast attempt and the pandas fallback, and the user's list stayed empty. The list/dict branches are now identity-preserving when unchanged, mirroring the existing object-ndarray branch.GroupBy.applyresult assembly (pandas parity, all verified empirically on 3.0.3)DataFrameGroupByresults return an empty frame keeping the value columns and dtypes (pandas GH9684/GH57775).sort, like pandas'_concat_objects; the finalsort_indexis removed since group-keyed results are already emitted in sorted key order and pandas preserves the UDF's within-group row order (GH52444).include_groups=TrueraisesValueError, matching pandas 3.0.Supporting fixes
DataFrame({"a": []})defaults untyped empty sequences to float64 like pandas' constructor (numpy's empty-array default);Series([])stays object.reset_indexderives the result columns dtype via pandas'Index.insertprovenance instead of re-inferring from the merged labels, andSeries.reset_indexresolves the value-column name before resetting (pandas'to_frame(name).reset_index()semantics).as_columnroutes stdlibdatetime/timedeltaelements through the pandas object path, so mixed datetime+non-datetime lists raiseMixedTypeErrorinstead of silently coercing.test_groupby_apply_return_col_from_dfand a stale conditional xfail intest_dataframe_assign_scalar— both now match pandas exactly.Validation
tests/groupby/test_apply.py: 28 failed → 135/137 pass (2 inherent, documented).NO_EXTERNAL_ONLY_APIS=1; dataframe/series/reshape/indexes/input_output sweep (70k+ tests) green.cudf.pandasproxy unit tests pass.Checklist