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

[REVIEW] Reorganize Pytest Config and Add Quick Run Option #3137

Merged
merged 3 commits into from
Nov 18, 2020

Conversation

mdemoret-nv
Copy link
Contributor

This PR partially addresses #3093 by reorganizing the conftest.py files and adding a new plugins folder with a Quick Run plugin.

Reorganizing like this has the added benefits:

  1. Follows the PyTest Guidelines
  2. Command line options can be used from the ./python folder
    1. Before, if you were not in the python/cuml/test folder and tried to use --run_stress it would generate an error
  3. Command line options now show up in the help:
python$ pytest -h
usage: pytest [options] [file_or_dir] [file_or_dir] [...]

positional arguments:
  file_or_dir
...
cuML Custom Options:
  --run_stress          Runs tests marked with 'stress'. These are the most intense tests that take the longest to run and are designed to stress the hardware's compute resources.
  --run_quality         Runs tests marked with 'quality'. These tests are more computationally intense than 'unit', but less than 'stress'
  --run_unit            Runs tests marked with 'unit'. These are the quickest tests that are focused on accuracy and correctness.
  1. Marker information can now be accessed from the command line:
python$ pytest --markers
@pytest.mark.unit: Quickest tests focused on accuracy and correctness

@pytest.mark.quality: More intense tests than unit with increased runtimes

@pytest.mark.stress: Longest running tests focused on stressing hardware compute resources

@pytest.mark.mg: Multi-GPU tests

@pytest.mark.memleak: Test that checks for memory leaks

@pytest.mark.no_bad_cuml_array_check: Test that should not check for bad CumlArray uses
  1. Additional plugins can be added in a well organized way (more on that later)
  2. There is no need to install cuml in developer mode. PyTest will find the local tests automatically.
  3. Better integration for IDE users
    1. Personally, I use VSCode and the pytest extension does not work well

Additionally, I have included the "Quick Run" plugin which significantly reduces the number of tests run. This is a good option when running locally where you want to hit most tests, but not do every permutation. Below is the help section:

Quick Run Plugin:
  --quick_run           Selecting this option will reduce the number of tests run by only picking tests if one of the parameters has never been seen before.

And when running with the current branch-0.17 it significantly reduces the number of tests:

$ pytest --co --quick_run --disable-warnings python
======================================================================================================================================== test session starts ========================================================================================================================================
platform linux -- Python 3.8.6, pytest-6.1.2, py-1.9.0, pluggy-0.13.1
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/mdemoret/Repos/rapids/cuml-dev2/python, configfile: pytest.ini
plugins: profiling-1.7.0, hypothesis-5.41.0, benchmark-3.2.3, asyncio-0.14.0, timeout-1.4.2, cov-2.10.1
collected 36066 items / 33828 deselected / 2238 selected

Currently this results in a 75% reduction in runtime while only reducing the code coverage by 0.9%

@mdemoret-nv mdemoret-nv requested a review from a team as a code owner November 13, 2020 20:10
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov-io
Copy link

codecov-io commented Nov 13, 2020

Codecov Report

Merging #3137 (e31908b) into branch-0.17 (77da916) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #3137      +/-   ##
===============================================
- Coverage        70.57%   70.50%   -0.07%     
===============================================
  Files              197      196       -1     
  Lines            15461    15442      -19     
===============================================
- Hits             10911    10887      -24     
- Misses            4550     4555       +5     
Impacted Files Coverage Δ
python/cuml/common/import_utils.py 55.43% <0.00%> (-3.27%) ⬇️
python/cuml/benchmark/bench_helper_funcs.py 71.00% <0.00%> (-2.00%) ⬇️
python/cuml/pytest_benchmarks/test_bench.py
...on/cuml/_thirdparty/sklearn/preprocessing/_data.py 63.54% <0.00%> (+0.08%) ⬆️
python/cuml/thirdparty_adapters/adapters.py 88.55% <0.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77da916...e31908b. Read the comment docs.

Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks great... one Python style thing would be to remember to omit parens around simple condition. Otherwise seems very clear. I only have small questions and style asks - can approve quickly, though I'd like to get a few more eyes on it since it changes tests a bit.

python/conftest.py Show resolved Hide resolved
python/conftest.py Outdated Show resolved Hide resolved
python/cuml/test/plugins/quick_run_plugin.py Show resolved Hide resolved
python/cuml/test/plugins/quick_run_plugin.py Outdated Show resolved Hide resolved
python/cuml/test/plugins/quick_run_plugin.py Show resolved Hide resolved
python/cuml/test/plugins/quick_run_plugin.py Show resolved Hide resolved
python/pytest.ini Show resolved Hide resolved
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Just a super quick look with 2 comments from my part

python/cuml/test/plugins/quick_run_plugin.py Outdated Show resolved Hide resolved
python/pytest.ini Show resolved Hide resolved
@mdemoret-nv mdemoret-nv added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Nov 17, 2020
@dantegd dantegd merged commit e377ae9 into rapidsai:branch-0.17 Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants