From 2ff0827ebec07a9f6596d9aca4e10d7f174240d5 Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 4 Jun 2026 20:48:42 +0000 Subject: [PATCH 1/3] fix --- .agents/skills/debug-cudf-pandas/SKILL.md | 34 ++++++++++++----------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/.agents/skills/debug-cudf-pandas/SKILL.md b/.agents/skills/debug-cudf-pandas/SKILL.md index 6d60e189038e..1c48d8e0ee78 100644 --- a/.agents/skills/debug-cudf-pandas/SKILL.md +++ b/.agents/skills/debug-cudf-pandas/SKILL.md @@ -14,7 +14,7 @@ When the pandas test suite is run with `-p cudf.pandas`, test failures indicate - A **proxy/dispatch bug** — the wrapping/unwrapping mechanism doesn't correctly handle a type or operation - A **missing proxy registration** — a pandas type or return value has no registered cudf equivalent - A **to/from_pandas conversion bug** — data is corrupted or lost when converting between cudf and pandas objects -- A **test setup bug** — the testing scripts or conftest-patch introduce an issue +- A **test setup bug** — the testing scripts or the pandas testing plugin introduce an issue - A **dependency/environment gap** — the test requires a package (e.g. xlsxwriter) that pandas CI has but our test environment lacks, causing a different code path to execute - A **pandas bug** — rarely, the expected behavior in the pandas test itself is wrong @@ -37,10 +37,13 @@ The following patterns are prohibited regardless of whether they make a test pas Before starting, verify you are at the repository root. All commands in this skill assume the working directory is the cudf repository root. -**Clean up any previous test run state.** The test runner appends `conftest-patch.py` to the pandas conftest on every invocation. If `pandas-testing/pandas-tests/` already exists from a prior run, the conftest will have duplicate hook registrations and cause spurious errors. Always delete it before running: +**Reusing or refreshing the test checkout.** The test harness lives in `pandas-testing/pandas-tests/`. The runner performs first-time setup (cloning pandas, copying the test tree, rewriting imports) *only* when the relevant directories are missing, so re-running reuses the existing checkout — it is safe and fast. The xfail/skip markers are applied by a pytest plugin loaded fresh on every run (`-p cudf.pandas.scripts.pandas-testing-plugin`), not by appending to the pandas `conftest.py`, so repeated runs no longer accumulate duplicate hook registrations. + +Delete the checkout only when you need a clean slate. The runner uses two independent guards: it clones pandas into `pandas-testing/pandas/` only if that directory is missing, and copies the test tree into `pandas-testing/pandas-tests/` only if *that* is missing. So removing just `pandas-tests/` re-copies a clean test tree **from the existing clone**, while picking up a new pandas version (the clone is pinned to the tag matching the installed pandas) requires removing the whole `pandas-testing/` so the clone is refetched: ```bash -rm -rf pandas-testing/pandas-tests/ +rm -rf pandas-testing/pandas-tests/ # re-copy tests from the existing clone +rm -rf pandas-testing/ # full reset, e.g. after a pandas version change ``` The cudf Python package is almost entirely pure Python. For **inplace installs** (e.g. `pip install -e .`), changes to `.py` files take effect immediately — no rebuild is needed. For non-inplace installs (e.g. `./build.sh`), you must either reinstall or copy changed files to site-packages. @@ -60,27 +63,27 @@ Node IDs with parameters like `[Float64-False-False-first]` target a specific pa --- -## Step 0 — Update conftest-patch.py +## Step 0 — Update pandas-testing-plugin.py -The file `python/cudf/cudf/pandas/scripts/conftest-patch.py` contains three dictionaries that gate how tests are handled: +The file `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py` contains three dictionaries that gate how tests are handled: - **`NODEIDS_THAT_FAIL`** — tests marked `xfail` (expected to fail). Keys are alphabetically sorted. - **`NODEIDS_TO_SKIP`** — tests marked `skip` (not run at all). Keys are alphabetically sorted. - **`NODEIDS_PATHS_TO_SKIP`** — prefix-based path skips covering entire modules. -Because the test runner sets `xfail_strict = true`, a test listed in `NODEIDS_THAT_FAIL` that unexpectedly *passes* is reported as `XPASS` — which is also a failure. You must remove the entry before testing your fix, or you will never see a genuine pass. +The pandas-tests harness runs with `xfail_strict = false` (set in the vendored `pandas-tests/pyproject.toml` to tolerate flaky XPASSes — [rapidsai/cudf#22681](https://github.com/rapidsai/cudf/issues/22681)). A test listed in `NODEIDS_THAT_FAIL` that now *passes* is therefore reported as `XPASS` **without failing the run**, so a stale entry will not flag itself. You must remove the entry yourself before testing your fix; otherwise the test reports `XPASS` instead of a genuine `PASSED` and the dead marker lingers silently. Search for the node ID: ```bash grep -n "tests/groupby/test_reductions.py::test_first_last_skipna" \ - python/cudf/cudf/pandas/scripts/conftest-patch.py + python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py ``` If found, remove the line. Keys must remain in alphabetical order after the edit. Then validate the file still parses: ```bash -python -c "exec(open('python/cudf/cudf/pandas/scripts/conftest-patch.py').read())" +python -c "exec(open('python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py').read())" ``` If the node ID is *not* found in any dictionary, you are dealing with a new regression — proceed directly to Step 1. @@ -92,15 +95,14 @@ If the node ID is *not* found in any dictionary, you are dealing with a new regr Run from the repo root: ```bash -rm -rf pandas-testing/pandas-tests/ bash python/cudf/cudf/pandas/scripts/run-pandas-tests.sh \ "tests/groupby/test_reductions.py::test_first_last_skipna[Float64-False-False-first]" \ -xvs ``` -The script clones the matching pandas version, copies tests, appends the conftest patch, and runs pytest with `-p cudf.pandas`. Substitute your actual node ID. +On the first run the script clones the matching pandas version and copies the test tree (subsequent runs reuse it). It runs pytest with both `-p cudf.pandas` and the marker plugin `-p cudf.pandas.scripts.pandas-testing-plugin`, and automatically applies `-m "not slow and not single_cpu and not db and not network"` and `--disable-warnings`. Substitute your actual node ID. -**If the test passes**: the xfail entry was stale. Commit only the `conftest-patch.py` change and stop. +**If the test passes**: the xfail entry was stale. Commit only the `pandas-testing-plugin.py` change and stop. **If the test fails**: read the failure output carefully — the assertion message tells you the exact behavioral difference. Proceed to Step 2. @@ -327,7 +329,7 @@ bash python/cudf/cudf/pandas/scripts/run-pandas-tests.sh \ "tests//" --tb=line -q ``` -Replace `` with the directory containing your test (e.g. `tests/groupby/`). Any new failures that are not already listed in `conftest-patch.py` must be investigated before committing. +Replace `` with the directory containing your test (e.g. `tests/groupby/`). Any new failures that are not already listed in `pandas-testing-plugin.py` must be investigated before committing. **d. Add unit tests (where appropriate):** @@ -345,7 +347,7 @@ Stage only the intended files — never anything from `pandas-testing/`: ```bash git add python/cudf/cudf/ # source fix (if applicable) -git add python/cudf/cudf/pandas/scripts/conftest-patch.py # xfail removal +git add python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py # xfail removal git status # verify nothing from pandas-testing/ is staged git commit -m "fix(cudf.pandas): @@ -374,13 +376,13 @@ For intentional divergence: stop and ask the user. In most cases, the goal is to - `mode.pandas_compatible` is automatically set to `True` when `cudf.pandas` is active. This is done at the end of `python/cudf/cudf/pandas/_wrappers/pandas.py`. - cudf Python is almost entirely pure Python — for inplace installs, changes to `.py` files take effect immediately without rebuilding. -- `pandas-testing/pandas-tests/` must be deleted before each test run to avoid duplicate conftest hook registrations (the script appends the patch file on every run). -- Keys in all three `conftest-patch.py` dictionaries must remain in alphabetical order. +- The xfail/skip markers are applied by a pytest plugin (`-p cudf.pandas.scripts.pandas-testing-plugin`) loaded fresh on every run, not by appending to the pandas `conftest.py`. Re-running with an existing `pandas-testing/pandas-tests/` is safe and fast. Deleting only `pandas-tests/` re-copies the test tree from the existing clone; to pick up a new pandas version delete the whole `pandas-testing/`, since the `pandas/` clone is guarded separately and is not refetched by removing `pandas-tests/` alone. +- Keys in all three `pandas-testing-plugin.py` dictionaries must remain in alphabetical order. - Never write comments that explain what old code was replaced — write comments about what the code does and why. - Never modify the pandas test files themselves — fix cudf, not pandas. - Never fix the testing APIs (like `assert_frame_equal`, `assert_series_equal`) — fix the actual APIs that produce wrong results. - First see if the problem is in cudf classic and fix it there; if not, then move over to cudf.pandas. -- Tests run with `xfail_strict = true` — a test listed in `NODEIDS_THAT_FAIL` that unexpectedly passes is reported as `XPASS` (also a failure). Remove from the list before testing. +- The pandas-tests harness runs with `xfail_strict = false` (vendored `pandas-tests/pyproject.toml`, to tolerate flaky XPASSes — issue #22681), so a stale `NODEIDS_THAT_FAIL` entry that now passes shows up as a non-failing `XPASS` and won't flag itself. Remove the entry yourself when your fix lands so you see a genuine `PASSED`. - When a fix requires adding a test dependency, update `dependencies.yaml` (under `test_cudf_pandas_pandas_tests` for conda environments) and run `rapids-dependency-file-generator` to propagate. Never manually edit the generated `pyproject.toml` entries marked as auto-generated. - Always verify vanilla pandas behavior before implementing proxy-layer fixes. If the test also fails without cudf.pandas, the problem is upstream or environmental, not a cudf bug. - xfail explanation strings should describe the root cause ("openpyxl limitation", "pandas test assumes xlsxwriter is installed"), not just the error type ("AssertionError", "IndexError"). From a47acc2dc4ce1dd89f6f97332d47c5c5cb63ec05 Mon Sep 17 00:00:00 2001 From: GALI PREM SAGAR Date: Thu, 4 Jun 2026 16:56:01 -0500 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com> --- .agents/skills/debug-cudf-pandas/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/debug-cudf-pandas/SKILL.md b/.agents/skills/debug-cudf-pandas/SKILL.md index 1c48d8e0ee78..56b36c194e0c 100644 --- a/.agents/skills/debug-cudf-pandas/SKILL.md +++ b/.agents/skills/debug-cudf-pandas/SKILL.md @@ -71,7 +71,7 @@ The file `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py` contains thr - **`NODEIDS_TO_SKIP`** — tests marked `skip` (not run at all). Keys are alphabetically sorted. - **`NODEIDS_PATHS_TO_SKIP`** — prefix-based path skips covering entire modules. -The pandas-tests harness runs with `xfail_strict = false` (set in the vendored `pandas-tests/pyproject.toml` to tolerate flaky XPASSes — [rapidsai/cudf#22681](https://github.com/rapidsai/cudf/issues/22681)). A test listed in `NODEIDS_THAT_FAIL` that now *passes* is therefore reported as `XPASS` **without failing the run**, so a stale entry will not flag itself. You must remove the entry yourself before testing your fix; otherwise the test reports `XPASS` instead of a genuine `PASSED` and the dead marker lingers silently. +The pandas-tests harness runs with `xfail_strict = false` (set in the vendored `pandas-tests/pyproject.toml` to tolerate flaky XPASSes — [rapidsai/cudf#22681](https://github.com/rapidsai/cudf/issues/22681)). A test listed in `NODEIDS_THAT_FAIL` that now *passes* is therefore reported as `XPASS` **without failing the run**, so a stale entry will not flag itself. You must change the false to true yourself before testing your fix; otherwise the test reports `XPASS` instead of a genuine `PASSED` and the dead marker lingers silently. Do not commit this change in any commit. Search for the node ID: From 580adb400f2e87a620b547d645eb9715b2451172 Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 4 Jun 2026 22:38:02 +0000 Subject: [PATCH 3/3] address reviews --- .agents/skills/debug-cudf-pandas/SKILL.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.agents/skills/debug-cudf-pandas/SKILL.md b/.agents/skills/debug-cudf-pandas/SKILL.md index 56b36c194e0c..e19fe214a4ae 100644 --- a/.agents/skills/debug-cudf-pandas/SKILL.md +++ b/.agents/skills/debug-cudf-pandas/SKILL.md @@ -37,13 +37,13 @@ The following patterns are prohibited regardless of whether they make a test pas Before starting, verify you are at the repository root. All commands in this skill assume the working directory is the cudf repository root. -**Reusing or refreshing the test checkout.** The test harness lives in `pandas-testing/pandas-tests/`. The runner performs first-time setup (cloning pandas, copying the test tree, rewriting imports) *only* when the relevant directories are missing, so re-running reuses the existing checkout — it is safe and fast. The xfail/skip markers are applied by a pytest plugin loaded fresh on every run (`-p cudf.pandas.scripts.pandas-testing-plugin`), not by appending to the pandas `conftest.py`, so repeated runs no longer accumulate duplicate hook registrations. +**Setting up and refreshing the test checkout.** The test harness lives in `pandas-testing/pandas-tests/`. The runner performs first-time setup (cloning pandas, copying the test tree, rewriting imports) *only* when the relevant directories are missing; once they exist it runs against them as-is and never refreshes them. The xfail/skip markers are applied by a pytest plugin loaded fresh on every run (`-p cudf.pandas.scripts.pandas-testing-plugin`), not by appending to the pandas `conftest.py`, so repeated runs do not accumulate duplicate hook registrations. -Delete the checkout only when you need a clean slate. The runner uses two independent guards: it clones pandas into `pandas-testing/pandas/` only if that directory is missing, and copies the test tree into `pandas-testing/pandas-tests/` only if *that* is missing. So removing just `pandas-tests/` re-copies a clean test tree **from the existing clone**, while picking up a new pandas version (the clone is pinned to the tag matching the installed pandas) requires removing the whole `pandas-testing/` so the clone is refetched: +Because the runner never refreshes an existing checkout, do not rely on its contents being current, and be aware that any edit you make under `pandas-testing/` persists into every subsequent run. Never modify the vendored pandas test files (the `tests/**.py` tree — see the "never modify the pandas test files" rule below); a stray edit there will silently follow you and can derail your investigation. The one sanctioned in-place edit is the temporary `xfail_strict` flip in `pandas-tests/pyproject.toml` described in Step 0 — revert it (or delete the checkout) once you are done so it does not linger. Delete the checkout whenever it may be stale. In particular, if you change `python/cudf/cudf/pandas/scripts/run-pandas-tests.sh` in a way that affects what it places in `pandas-testing/`, the existing checkout will not reflect that change — delete it before re-running so the runner rebuilds it. The runner uses two independent guards: it clones pandas into `pandas-testing/pandas/` only if that directory is missing, and copies the test tree into `pandas-testing/pandas-tests/` only if *that* is missing. So removing just `pandas-tests/` re-copies a clean test tree **from the existing clone**, while picking up a new pandas version (the clone is pinned to the tag matching the installed pandas) requires removing the whole `pandas-testing/` so the clone is refetched: ```bash -rm -rf pandas-testing/pandas-tests/ # re-copy tests from the existing clone -rm -rf pandas-testing/ # full reset, e.g. after a pandas version change +rm -rf pandas-testing/pandas-tests/ # re-copy a clean test tree from the existing clone +rm -rf pandas-testing/ # full reset, e.g. after a pandas version change or a change to run-pandas-tests.sh ``` The cudf Python package is almost entirely pure Python. For **inplace installs** (e.g. `pip install -e .`), changes to `.py` files take effect immediately — no rebuild is needed. For non-inplace installs (e.g. `./build.sh`), you must either reinstall or copy changed files to site-packages. @@ -376,13 +376,13 @@ For intentional divergence: stop and ask the user. In most cases, the goal is to - `mode.pandas_compatible` is automatically set to `True` when `cudf.pandas` is active. This is done at the end of `python/cudf/cudf/pandas/_wrappers/pandas.py`. - cudf Python is almost entirely pure Python — for inplace installs, changes to `.py` files take effect immediately without rebuilding. -- The xfail/skip markers are applied by a pytest plugin (`-p cudf.pandas.scripts.pandas-testing-plugin`) loaded fresh on every run, not by appending to the pandas `conftest.py`. Re-running with an existing `pandas-testing/pandas-tests/` is safe and fast. Deleting only `pandas-tests/` re-copies the test tree from the existing clone; to pick up a new pandas version delete the whole `pandas-testing/`, since the `pandas/` clone is guarded separately and is not refetched by removing `pandas-tests/` alone. +- The xfail/skip markers are applied by a pytest plugin (`-p cudf.pandas.scripts.pandas-testing-plugin`) loaded fresh on every run, not by appending to the pandas `conftest.py`. The runner sets up `pandas-testing/` only when it is missing and never refreshes it afterward, so if you modify `python/cudf/cudf/pandas/scripts/run-pandas-tests.sh` in a way that changes the `pandas-testing/` directory, delete it before re-running the script. Deleting only `pandas-tests/` re-copies the test tree from the existing clone; to pick up a new pandas version delete the whole `pandas-testing/`, since the `pandas/` clone is guarded separately and is not refetched by removing `pandas-tests/` alone. - Keys in all three `pandas-testing-plugin.py` dictionaries must remain in alphabetical order. - Never write comments that explain what old code was replaced — write comments about what the code does and why. - Never modify the pandas test files themselves — fix cudf, not pandas. - Never fix the testing APIs (like `assert_frame_equal`, `assert_series_equal`) — fix the actual APIs that produce wrong results. - First see if the problem is in cudf classic and fix it there; if not, then move over to cudf.pandas. -- The pandas-tests harness runs with `xfail_strict = false` (vendored `pandas-tests/pyproject.toml`, to tolerate flaky XPASSes — issue #22681), so a stale `NODEIDS_THAT_FAIL` entry that now passes shows up as a non-failing `XPASS` and won't flag itself. Remove the entry yourself when your fix lands so you see a genuine `PASSED`. +- The pandas-tests harness runs with `xfail_strict = false` (vendored `pandas-tests/pyproject.toml`, to tolerate flaky XPASSes — issue #22681), so a stale `NODEIDS_THAT_FAIL` entry that now passes shows up as a non-failing `XPASS` and won't flag itself. While testing your fix, flip `xfail_strict` to `true` in that file so the `XPASS` surfaces (do not commit that change) and remove the stale entry so the test reports a genuine `PASSED`. - When a fix requires adding a test dependency, update `dependencies.yaml` (under `test_cudf_pandas_pandas_tests` for conda environments) and run `rapids-dependency-file-generator` to propagate. Never manually edit the generated `pyproject.toml` entries marked as auto-generated. - Always verify vanilla pandas behavior before implementing proxy-layer fixes. If the test also fails without cudf.pandas, the problem is upstream or environmental, not a cudf bug. - xfail explanation strings should describe the root cause ("openpyxl limitation", "pandas test assumes xlsxwriter is installed"), not just the error type ("AssertionError", "IndexError").