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

[FEA] Reorganize and improve Python tests #9999

Open
5 of 12 tasks
vyasr opened this issue Jan 7, 2022 · 2 comments
Open
5 of 12 tasks

[FEA] Reorganize and improve Python tests #9999

vyasr opened this issue Jan 7, 2022 · 2 comments
Labels
cuDF (Python) Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change proposal Change current process or code tests Unit testing for project

Comments

@vyasr
Copy link
Contributor

vyasr commented Jan 7, 2022

Note for developers
This is a meta-issue aiming to categorize a wide range of issues. Developers who want to tackle a specific item from the checklist below should create a new issue for just that item, self-assign that issue, and then link it to the checklist above.

Is your feature request related to a problem? Please describe.
There are currently a number of different problems that make it difficult to find, add, or run tests.

  • Tests are partially and inconsistently organized by functionality, data types, and parametrizations, so it's not clear which file tests of a specific function might be in. The partial organization by dtype is particularly confusing because it means that in some files we test a single function across many dtypes, whereas in other files we test many functions for a single dtype.
  • Test files are too large and contain too many tests.
  • There are currently many tests that raise warnings as well as many xfailed tests that actually xpass.
  • Tests are currently slow to run because we rely on excessive parametrization and we test many private APIs. Additionally, cuIO tests are especially slow because the corresponding libcudf APIs are difficult to test, so a greater burden is placed on the Python APIs to capture a wider range of issues.

Describe the solution you'd like
These are tasks that we propose to undertake to address the various issues discussed above:

  • See Remove xfail condition from now passing tests #7386. Change pytest to run fail on xpasses by specifying the strict parameter. Here are the relevant pytest docs.
  • Make pytest fail when it encounters warnings. We can accomplish this by setting in the appropriate config file (probably setup.cfg). (Done in Raise warnings as errors in the test suite #12468)
  • Reorganize test files to match the groupings in our API documentation. See also [QST/Discussion] Reorganize pytests #4730
  • Reorganize tests into classes that reflect the actual class hierarchies. For example, tests that should be performed for Series and DataFrame objects can go into a test_indexed_frame.py file.
  • Reduce excessive parametrization. Many individual tests are actually run hundreds or even thousands of times because we construct a matrix of parameters. In many cases those parametrizations aren't actually testing anything useful, for instance when a test is parametrized by input size. In cases where we're worried about how the underlying libcudf algorithm behaves when block sizes are exceeded, for instance, we should push those tests down to C++. Python tests should favor fewer parameters and in some cases more specialized tests to handle specific edge cases.
  • Where appropriate, we should replace parametrization with fixtures. Fixtures will be typically evaluated lazily unlike parameters, which are materialized when the decorated functions are defined, so changing this should improve collection time and reduce the frequency with which we see failures during collection. Additionally, it makes it easier to debug failing tests when breakpoints are placed in the code; we don't want to hit breakpoints during collection. [FEA] Create collection of dataframes for testing #2530 is related to this and may be resolved by addressing this task, but it's not quite as broad.
  • Remove tests of private functions/classes. If those functions/classes have corresponding public APIs, we need to make sure that those APIs are tested. If they are, then the tests of private code may be removed, otherwise those tests should be rewritten in terms of the corresponding public APIs.
  • Stop using pd._testing ([FEA] Figure out alternative to pandas._testing #6435)
  • Reduce runtime of I/O tests #10001
  • Implement standard fixtures or use a similar approach to ensure consistent dtype coverage in tests. As part of this, decide what constitutes sufficient dtype coverage
@vyasr vyasr added proposal Change current process or code tests Unit testing for project code quality cuDF (Python) Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 7, 2022
@vyasr vyasr added this to the CuDF Python Refactoring milestone Jan 7, 2022
@vyasr vyasr added this to Needs prioritizing in Other Issues via automation Jan 7, 2022
@vyasr vyasr added this to Issue-Needs prioritizing in v22.04 Release via automation Jan 7, 2022
@bdice
Copy link
Contributor

bdice commented Jan 7, 2022

I have started looking into xfail_strict=true in PR #9998. That will require some significant work to analyze all the xfail tests that pass (1956 tests failed with xfail_strict=true, and most of those are xpassed strict failures). I am going to prioritize solving locally failing tests like #7314 and reduce the number of warnings in the pytest log first, before going back to #9998.

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

rapids-bot bot pushed a commit that referenced this issue Feb 15, 2022
This PR reduces the overall runtime of the cuDF pytest suite. Changes include:

- asserting equal on the GPU where possible for large datasets
- in some cases reducing excessive test data size

part of #9999

Authors:
  - https://github.com/brandon-b-miller

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Ashwin Srinath (https://github.com/shwina)
  - Bradley Dice (https://github.com/bdice)

URL: #10203
@caryr35 caryr35 removed this from Issue-Needs prioritizing in v22.04 Release Apr 12, 2022
@caryr35 caryr35 added this to Issue-Needs prioritizing in v22.06 Release via automation Apr 12, 2022
@caryr35 caryr35 removed this from Issue-Needs prioritizing in v22.06 Release Jun 16, 2022
@caryr35 caryr35 added this to Issue-Needs prioritizing in v22.08 Release via automation Jun 16, 2022
@caryr35 caryr35 removed this from Issue-Needs prioritizing in v22.08 Release Aug 11, 2022
@caryr35 caryr35 added this to Issue-Needs prioritizing in v22.10 Release via automation Aug 11, 2022
@caryr35 caryr35 removed this from Issue-Needs prioritizing in v22.10 Release Oct 18, 2022
@caryr35 caryr35 added this to Issue-Needs prioritizing in v22.12 Release via automation Oct 18, 2022
@shwina shwina removed this from Issue-Needs prioritizing in v22.12 Release Oct 24, 2022
rapids-bot bot pushed a commit that referenced this issue Dec 5, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Ashwin Srinath (https://github.com/shwina)

URL: #12293
rapids-bot bot pushed a commit that referenced this issue Dec 5, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Michael Wang (https://github.com/isVoid)

URL: #12305
rapids-bot bot pushed a commit that referenced this issue Dec 6, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - https://github.com/brandon-b-miller
  - Matthew Roeschke (https://github.com/mroeschke)

URL: #12304
rapids-bot bot pushed a commit that referenced this issue Dec 6, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Ashwin Srinath (https://github.com/shwina)

URL: #12310
rapids-bot bot pushed a commit that referenced this issue Dec 7, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Lawrence Mitchell (https://github.com/wence-)

URL: #12326
rapids-bot bot pushed a commit that referenced this issue Dec 9, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #12313
rapids-bot bot pushed a commit that referenced this issue Dec 10, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #12324
rapids-bot bot pushed a commit that referenced this issue Dec 10, 2022
One note with these deprecations. pandas has a special value used for parameters that are defaulted, and as a result explicitly passing None will throw a warning. We don't do this, hence the difference in the warnings contexts I use for our calls vs pandas (`pytest.warns` vs `expect_warning_if`). I didn't feel like this was worth commenting on in every place, but I can if reviewers want.

Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #12334
rapids-bot bot pushed a commit that referenced this issue Dec 13, 2022
I realized that my previous warning reduction PRs were causing some circular work where I would add a new warning to cudf to match pandas, which would cause those new warnings to appear in modules that I had previously declared free of warnings. To prevent this, I've changed my approach to instead go through the test modules in alphabetical order and ensure that they are all error free up to that point. This PR removes warnings from all test modules up to test_dataframe.py.

Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #12355
rapids-bot bot pushed a commit that referenced this issue Dec 15, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Matthew Roeschke (https://github.com/mroeschke)

URL: #12381
rapids-bot bot pushed a commit that referenced this issue Dec 19, 2022
Contributes to #9999 and #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Matthew Roeschke (https://github.com/mroeschke)
  - Ashwin Srinath (https://github.com/shwina)

URL: #12369
rapids-bot bot pushed a commit that referenced this issue Dec 21, 2022
Contributes to #9999 and #10363.

When I merge these changes with #12369 I no longer see any warnings on my machine. I suspect that there will be slightly different results on different machines, so we'll see have to see how CI looks after both PRs are merged before we close #10363.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Matthew Roeschke (https://github.com/mroeschke)
  - Bradley Dice (https://github.com/bdice)

URL: #12406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuDF (Python) Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change proposal Change current process or code tests Unit testing for project
Projects
Status: In Progress
Other Issues
Needs prioritizing
Development

No branches or pull requests

2 participants