Skip to content
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 benchmarks compatibility with newer pytest-cases #14764

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

jameslamb
Copy link
Member

Description

Reverts changes from #14756.

  • updates cudf's tests to be compatible with the latest pytest-cases (version 3.8.2)
  • puts a floor of pytest-cases>=3.8.2 on that project to be sure older versions aren't used

Checklist

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

Notes for Reviewers

The fix here was to stop using pytest-cases's automatic collection of cases, and to instead explicitly tell it where to look.

Per the pytest_cases.get_all_cases() docs (docs link)

cases:
a case function, a class containing cases, a module or a module name string (relative module names accepted).
...
When a module is listed, all of its functions matching the prefix, filter and has_tag are selected, including those functions nested in classes following naming pattern *Case*.

Since there are so few uses of pytest_cases in this project, I think explicitly passing module names is preferable to adjusting the repo to work with the new patterns introduced in smarie/python-pytest-cases#320.

For clarity, I'm also proposing renaming files that only contain test cases such that they begin with cases_*, but this isn't strictly necessary... happy to revert that if you'd like.

How I tested this

following CONTRIBUTING.md (click me)

Based on the advice in https://docs.rapids.ai/resources/reproducing-ci/, pointed build scripts at local output directories.

sed -ri '/rapids-download-conda-from-s3/ s/_CHANNEL=.*/_CHANNEL=${RAPIDS_CONDA_BLD_OUTPUT_DIR}/' ci/*.sh

Then re-generated the dependency files.

rapids-dependency-file-generator --clean

Then created a conda environment to run tests in and built the library.

conda env create \
  --name cudf-dev \
  --file ./conda/environments/all_cuda-120_arch-x86_64.yaml

source activate cudf-dev

./ci/build.sh libcudf cudf

Then ran the benchmarks, following

# Run benchmarks with both cudf and pandas to ensure compatibility is maintained.

pushd python/cudf

rm -rf ./.pytest_cache

CUDF_BENCHMARKS_DEBUG_ONLY=ON \
pytest \
  --cache-clear \
  --numprocesses=16 \
  --dist=loadscope \
  --cov-config=.coveragerc \
  --cov=cudf \
  --cov-report=term \
  benchmarks

rm -rf ./.pytest_cache

CUDF_BENCHMARKS_USE_PANDAS=ON \
CUDF_BENCHMARKS_DEBUG_ONLY=ON \
pytest \
  --cache-clear \
  --numprocesses=8 \
  --dist=loadscope \
  --cov-config=.coveragerc \
  --cov=cudf \
  --cov-report=term \
  benchmarks

popd

Saw those tests succeed with my changes, and fail like #14712 (comment) without them.

I also tried adding a raise RuntimeError() to the test cases in e.g. cases_dataframe.py, just to convince myself that the benchmark cases were actually being successfully collected and run.

@github-actions github-actions bot added Python Affects Python cuDF API. conda labels Jan 16, 2024
@jameslamb jameslamb added 2 - In Progress Currently a work in progress 5 - DO NOT MERGE Hold off on merging; see PR for details tests Unit testing for project non-breaking Non-breaking change and removed Python Affects Python cuDF API. conda labels Jan 16, 2024
@jameslamb jameslamb marked this pull request as ready for review January 16, 2024 19:49
@jameslamb jameslamb requested review from a team as code owners January 16, 2024 19:49
@jameslamb jameslamb added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress 5 - DO NOT MERGE Hold off on merging; see PR for details labels Jan 16, 2024
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@github-actions github-actions bot added Python Affects Python cuDF API. conda labels Jan 16, 2024
@mroeschke mroeschke added the improvement Improvement / enhancement to an existing function label Jan 17, 2024
Copy link
Contributor

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM

@bdice
Copy link
Contributor

bdice commented Jan 18, 2024

/merge

@rapids-bot rapids-bot bot merged commit eeee795 into rapidsai:branch-24.02 Jan 18, 2024
67 checks passed
@jameslamb jameslamb deleted the pytest-cases branch January 22, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API. tests Unit testing for project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants