Re-derive cudf.pandas intermediate proxies when their parents change - #22998
Conversation
An `_IntermediateProxy` (e.g. a groupby/rolling/accessor) cached its wrapped object at creation. When a parent proxy (the originating frame/series, or a proxy passed as an argument such as a grouping key) was mutated or replaced afterwards, the intermediate kept returning the stale snapshot instead of reflecting the live parent. Snapshot the identity of each parent proxy's wrapped object and, on conversion, re-derive the intermediate from the originating call when a parent has changed. Parent collection is iterative (explicit stack) to avoid a self-referential closure cycle that would delay reference-counted teardown relied on by e.g. the pandas Series.str-accessor test. Split out of NVIDIA#22903.
Per review, split the `_IntermediateProxy` parent-staleness fix (and its unit test plus the iloc xfail removals it unblocks) out of this PR; it now lives in NVIDIA#22998. This PR keeps only the DataFrame/GroupBy value_counts changes.
|
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:
📝 WalkthroughWalkthroughThe proxy layer now tracks parent proxies for intermediate results, re-derives cached wrapped objects when parents change, and restores that tracking after unpickling. New regression tests cover groupby behavior after parent mutation, and the pandas-testing-plugin drops two failure-reason entries. ChangesProxy parent staleness tracking
Pandas testing plugin mapping cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 424-431: The fast-path in fast_slow_proxy._fsproxy_slow_to_fast()
refreshes the derived object when _fsproxy_parents_changed() is true, but it
only updates the parent snapshot and leaves _fsproxy_wrapped stale. Update the
cached wrapped value to the newly derived result before returning, so subsequent
calls on the same proxy continue to use the refreshed object instead of falling
back to an outdated cache.
🪄 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: 95affa84-66ff-46a8-9c60-e44dd83a96a4
📒 Files selected for processing (3)
python/cudf/cudf/pandas/fast_slow_proxy.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf_pandas_tests/test_cudf_pandas.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test 13eca13 |
…diate-proxy-staleness
…diate-proxy-staleness
…diate-proxy-staleness
|
/okay to test 78a899e |
vyasr
left a comment
There was a problem hiding this comment.
Couple of small questions but the implementation seems fine to me.
…diate-proxy-staleness
|
/merge |
Split out of #22903 per review.
Problem
An
_IntermediateProxy(e.g. agroupby,rolling, or accessor object) caches its wrapped object at creation time. When a parent proxy (the originating frame/series, or a proxy passed as an argument such as a grouping key) is mutated or replaced after the intermediate is created, the intermediate keeps returning the stale snapshot instead of reflecting the live parent.For example, a column added to a frame after a groupby was created is not visible through that groupby, which diverges from pandas' live-reference semantics:
Fix
Snapshot the identity of each parent proxy's wrapped object when the intermediate is created, and on
_fsproxy_slow_to_fast/_fsproxy_fast_to_slowre-derive the intermediate from the originating call when a parent has changed.Parent collection (
_collect_parent_proxies) is intentionally iterative (an explicit stack) rather than a recursive closure: a recursive closure would hold a cell referencing itself, forming a reference cycle that delays reference-counted teardown and breaks code relying on prompt teardown (e.g. the pandasSeries.str-accessor circular-reference test).Tests
Adds
test_groupby_reflects_parent_frame_mutation, and removes the now-passingtest_iloc_setitem_axis_argument[True/False]xfails from the pandas-tests plugin.