-
Notifications
You must be signed in to change notification settings - Fork 915
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
Fix CI workflows for pandas-tests and add test summary. #14847
Conversation
/merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I'd like to see changed, but this PR generally looks good and I don't think it's worth blocking merge if you don't want to make the change, so resolve as you see fit.
python -m pytest \ | ||
--cache-clear \ | ||
--junitxml="${RAPIDS_TESTS_DIR}/junit-cudf.xml" \ | ||
--numprocesses=8 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you choose to use --numprocesses
instead of -n
to match the conda tests? I'm fine with either option but I think (without having done any real investigation) that -n
is more common. That may also be a dichotomy between wheels/conda (since we implemented GHA for each in parallel at roughly the same time there was less coordination/enforcement of uniformity) that we could try to unify over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've generally chosen to write the full names of flags wherever possible in our CI scripts. You're correct, this was an artifact of us doing isolated work on conda and wheels at the same time. I took this opportunity to make them match.
|
||
# Grab the Pandas source corresponding to the version | ||
# of Pandas installed. | ||
PANDAS_VERSION=$(python -c "import pandas; print(pandas.__version__)") | ||
|
||
PYTEST_IGNORES="--ignore=tests/io/test_user_agent.py" | ||
PYTEST_IGNORES="--ignore=tests/io/test_user_agent.py --ignore=tests/interchange/test_impl.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just inline this variable? It's only used once, it's defined far from that location (the pytest call), and it's incongruous with how we provide basically every other argument inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "far" in lines of code but not really that far semantically. It's just that there's another very large variable definition in between. I think I'm going to leave this as-is because I don't think it is worth another CI run to change.
Description
This PR fixes issues with the
pandas-tests
job that were introduced during the pandas 2 migration.It also closes #14846 by adding GitHub Actions summaries for all wheel test jobs, including
cudf.pandas
. Depends on rapidsai/shared-workflows#173.Checklist