Skip to content

Remove pytest pin #19127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 32 commits into
base: branch-25.08
Choose a base branch
from
Open

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jun 10, 2025

Description

Contributes to rapidsai/build-planning#105.

Checklist

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

@vyasr vyasr self-assigned this Jun 10, 2025
@vyasr vyasr requested a review from a team as a code owner June 10, 2025 17:29
@vyasr vyasr requested a review from AyodeAwe June 10, 2025 17:29
@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 10, 2025
@github-actions github-actions bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars pylibcudf Issues specific to the pylibcudf package labels Jun 10, 2025
@GPUtester GPUtester moved this to In Progress in cuDF Python Jun 10, 2025
@vyasr
Copy link
Contributor Author

vyasr commented Jun 10, 2025

The failure I see here looks like pytest-dev/pytest-cov#693 (cf. pytest-dev/pytest-xdist#1211).

@vyasr vyasr requested a review from a team as a code owner June 10, 2025 22:39
@vyasr vyasr requested review from wence- and rjzamora June 10, 2025 22:39
@vyasr
Copy link
Contributor Author

vyasr commented Jun 11, 2025

I resolved the above issue by ignoring one error. The other failure looks to be due to our testing of the streaming executor for cudf-polars using dask in some way that is conflicting with the latest versions of pytest and xdist.

@gforsyth
Copy link
Contributor

Going to note down a few things here that should not block this PR, but we might want to track moving forward (I'll open issues once I'm sure I've got a handle on all of this):

  1. Many of the test runs in ci/test_python_cudf.sh pass the --cov-config argument onward to run_cudf_pytests.sh. It is not clear to me if all of these coveragerc files even exist, and there's not an immediately apparent way to determine which file (if any) is being used to set coverage settings for a given subproject.

This is made trickier because coverage will use the first vaild config file that it finds, starting with .coveragerc, then setup.cfg, tox.ini, then pyproject.toml.

It would be good to standardize on one choice (probably pyproject.toml) except for those cases where we deliberately need to override settings for a particular test run.

gforsyth added 2 commits June 11, 2025 15:52
The coveragerc file already exists and will supercede any settings in
the `pyproject.toml`, so don't put extra config there.
@vyasr
Copy link
Contributor Author

vyasr commented Jun 12, 2025

One of the remaining issues is that it seems like more recent versions of pytest are making it harder to guarantee that the current path is not added to sys.path. Even when I use --import-mode=importlib CI is failing for pylibcudf tests because some files from the source tree for pylibcudf are being imported instead of the versions in the install tree. --import-mode=append gets past this issue, but then fails for us because pylibcudf has multiple test modules with the same name in different subdirectories. I think moving the tests out of the pylibcudf package directory is the best way to avoid this problem altogether.

@gforsyth
Copy link
Contributor

@vyasr -- I think I've got all the pytest errors squashed here. The pandas-test job is still failing, but looks more like legitimate test failures (although I don't know why)

@github-actions github-actions bot added the cudf.pandas Issues specific to cudf.pandas label Jun 16, 2025
@vyasr
Copy link
Contributor Author

vyasr commented Jun 16, 2025

After some offline discussion I added a few extra tests to the list of expected failures for pandas. It seems likely that the pandas version we are currently using has some issues with the latest pytest version. Those issues are likely fixed in the latest pandas release, so we can revisit these when we tackle #19099.

@@ -6,6 +6,6 @@ set -euo pipefail
# It is essential to cd into python/pylibcudf/pylibcudf as `pytest-xdist` + `coverage` seem to work only at this directory level.

# Support invoking run_cudf_pytests.sh outside the script directory
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/pylibcudf/pylibcudf/
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/pylibcudf/ || exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Is the || exit 1 here intentional? I see set -e above, so it shouldn't be necessary.

@@ -2,7 +2,7 @@
# Copyright (c) 2022-2025, NVIDIA CORPORATION.

# Support invoking test_python_cudf.sh outside the script directory
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../;
cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../ || exit 1
Copy link
Member

@KyleFromNVIDIA KyleFromNVIDIA Jun 24, 2025

Choose a reason for hiding this comment

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

Same question about || exit 1.

I see in this case set -euo pipefail is not being set above. We should set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to Kyle's question, does set -eou pipefail work the same as set -euo pipefail? I figured -o pipefail had to be together in the option set.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, it doesn't -- I would expect that to error, but given that this is here, it might just ignore the pipefaile entirely 😬

Copy link
Contributor

@bdice bdice Jun 25, 2025

Choose a reason for hiding this comment

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

Ceci n’est pas une pipefaile, as they say

@gforsyth
Copy link
Contributor

I can push up a few changes to address the review comments (and fix the bad pipefail)

@vyasr vyasr requested review from bdice and KyleFromNVIDIA June 25, 2025 02:17
@bdice
Copy link
Contributor

bdice commented Jun 25, 2025

Going to merge this once CI passes again, because it moves a lot of code and is likely to have merge conflicts (like the one I just resolved).

@bdice
Copy link
Contributor

bdice commented Jun 25, 2025

/merge

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

Successfully merging this pull request may close these issues.

5 participants