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.

Sign up for GitHub

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

Jump to bottom

[WIP] Nightly automated benchmark #4414

Merged
merged 43 commits into from
Feb 9, 2022
Merged

[WIP] Nightly automated benchmark #4414

merged 43 commits into from
Feb 9, 2022

Conversation

viclafargue
Copy link
Contributor

This PR contains the code allowing the nightly automated runs of benchmarks for cuML.

Nanthini10 and others added 22 commits October 28, 2021 20:31
@Nanthini10
initial commit
c5a0eae
@Nanthini10
Working initial LinReg benchmark with rapids-pytest
826f6ff
@Nanthini10
Add more
ba52785
@Nanthini10
ADD test_bench.py to this file and remove that; organisation
6c49918
@Nanthini10
ADD separate becnh files
f1d2248
@Nanthini10
Merge remote-tracking branch 'upstream/branch-21.12' into enh-auto-be…
a569fe0 
…nchmark
@viclafargue
Adding MNMG benchmarking
b309bcb
@Nanthini10
ADD more tests
72d194b
@viclafargue
Fix MNMG benchmarks
9206857
@viclafargue
Manual merge with Nanthini's branch
c869a6d
@viclafargue
Merge branch 'branch-21.12' into enh-auto-benchmark
ae37a65
@Nanthini10
Merge remote-tracking branch 'upstream/branch-21.12' into enh-auto-be…
e05e385 
…nchmark
@viclafargue
Enabling automated NVTX benchmarking
1543a11
@viclafargue
Fix algorithm
747a717
@viclafargue
Add license
95a250e
@Nanthini10
Increase test sizes
89c2475
@viclafargue
Training + Inference
ef06b96
@viclafargue
Fix for MNMG DBSCAN and MNMG UMAP bench
f6395e0
@viclafargue
Fix preprocessing + improve datagen
345ae39
@viclafargue
Rename run_benchmarks.py in automated dir
b6dd62f
@viclafargue
Add sklearn-intelex benchmark
eb0357a
@viclafargue
Merge branch 'branch-21.12' into enh-auto-benchmark
40f694d
@viclafargue viclafargue requested a review from a team as a code owner December 1, 2021 14:46
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Dec 1, 2021
dantegd
dantegd requested changes Dec 1, 2021
View reviewed changes
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.

Had a few initial comments

python/cuml/benchmark/automated/pytest.ini Show resolved Hide resolved
python/cuml/benchmark/automated/python_pytest_based/bench_cuml_classification.py Outdated Show resolved Hide resolved
python/cuml/benchmark/automated/python_pytest_based/datagen.py Outdated Show resolved Hide resolved
@caryr35 caryr35 added this to PR-WIP in v22.02 Release via automation Dec 8, 2021
@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 13, 2022
dantegd
dantegd requested changes Jan 13, 2022
View reviewed changes
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 have one question before we merge

python/cuml/benchmark/algorithms.py Outdated
@@ -97,7 +101,7 @@ def __init__(
cpu_data_prep_hook=None,
cuml_data_prep_hook=None,
accuracy_function=None,
bench_func=fit,
bench_func=fit_transform,
Copy link
Member

Choose a reason for hiding this comment

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

Question: Why change the default to fit_transform? What if I want to benchmark only training without inference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was simply for benchmark to cover both training and inference by default. This way, regressions on the inference side won't be missed. This is however, still possible to parameterize an AlgorithmPair with an other function to benchmark. Please tell me if you would like to revert that change.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to revert that change and if possible divide training and inference, will make triaging regressions easier and each benchmark to be as simple as possible while still being useful

v22.02 Release automation moved this from PR-WIP to PR-Needs review Jan 13, 2022
@ajschmidt8 ajschmidt8 removed the request for review from a team January 18, 2022 19:23
@ajschmidt8
Copy link
Member

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

@dantegd
Copy link
Member

dantegd commented Jan 24, 2022

rerun tests

@viclafargue viclafargue changed the base branch from branch-22.02 to branch-22.04 January 28, 2022 16:30
@caryr35 caryr35 added this to PR-WIP in v22.04 Release via automation Feb 7, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.04 Release Feb 7, 2022
@caryr35 caryr35 removed this from PR-Needs review in v22.02 Release Feb 7, 2022
@tfeher tfeher mentioned this pull request Feb 9, 2022
Open
@dantegd
Copy link
Member

dantegd commented Feb 9, 2022

rerun tests

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.04@3ccf77f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.04    #4414   +/-   ##
===============================================
  Coverage                ?   84.32%           
===============================================
  Files                   ?      250           
  Lines                   ?    20421           
  Branches                ?        0           
===============================================
  Hits                    ?    17220           
  Misses                  ?     3201           
  Partials                ?        0
Flag Coverage Δ
dask 45.15% <0.00%> (?)
non-dask 77.56% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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 3ccf77f...7e488ee. Read the comment docs.

v22.04 Release automation moved this from PR-Needs review to PR-Reviewer approved Feb 9, 2022
@dantegd
Copy link
Member

dantegd commented Feb 9, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1dd32dc into rapidsai:branch-22.04 Feb 9, 2022
v22.04 Release automation moved this from PR-Reviewer approved to Done Feb 9, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@viclafargue
Nightly automated benchmark (rapidsai#4414)
e253468 
This PR contains the code allowing the nightly automated runs of benchmarks for `cuML`.

Authors:
  - Victor Lafargue (https://github.com/viclafargue)
  - Nanthini (https://github.com/Nanthini10)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantegd dantegd dantegd approved these changes

Assignees
No one assigned
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v22.04 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@viclafargue @ajschmidt8 @dantegd @codecov-commenter @Nanthini10