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

Continous Benchmarking Action #1137

Merged
merged 24 commits into from
Apr 27, 2023
Merged

Conversation

JuanPedroGHM
Copy link
Member

@JuanPedroGHM JuanPedroGHM commented Mar 31, 2023

Description

Enables performance tracking of HeAT by using Action for Continuous Benchmarking

Issue/s resolved: #

Changes proposed:

  • Moved old benchmarks to benchmarks/old
  • Created new benchmarks meant to run on the action inside of benchmarks/cb
  • Added perun as an optional dependency
  • CB action runs on every push of a pull requests with the PR talk tag
  • Introduced dedicated create_spherical_dataset method in the utils module that can be used in unit tests and benchmarks

Type of change

  • New Github workflow for pull requests added.
  • New benchmarks organization

Performance

  • perun works with MPI, and won't print duplicate results when running on multiple processes.

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Title of PR is suitable for corresponding CHANGELOG entry

Does this change modify the behaviour of other functions? If so, which?

no

@ghost
Copy link

ghost commented Mar 31, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@github-actions
Copy link
Contributor

Thank you for the PR!

1 similar comment
@github-actions
Copy link
Contributor

Thank you for the PR!

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #1137 (7fe7a21) into main (9f6b2eb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1137      +/-   ##
==========================================
+ Coverage   91.79%   91.81%   +0.01%     
==========================================
  Files          72       73       +1     
  Lines       10497    10518      +21     
==========================================
+ Hits         9636     9657      +21     
  Misses        861      861              
Flag Coverage Δ
unit 91.81% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
heat/utils/data/__init__.py 100.00% <100.00%> (ø)
heat/utils/data/spherical.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

10 similar comments
@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Thank you for the PR!

mrfh92
mrfh92 previously approved these changes Apr 18, 2023
Copy link
Collaborator

@mrfh92 mrfh92 left a comment

Choose a reason for hiding this comment

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

Review for PR #1137 "Continuous Benchmarking Action"

This PR adds the possibility of continuous performance monitoring to Heat and therefore it is a highly valuable contribution to the overall functionality.

As far as I understand, there are three main changes coming along with this PR: First, those things required for the actual continuous benchmarking (including examples covering clustering and linear algebra); second, the "old" benchmarks have been moved to make place for the "new" continuous benchmarks; third, the unit tests for clustering have been cleaned up by introducing a dedicated function for creating the spherical test data set that now can also be used for benchmarking purposes.

As far as I can judge, the code is correct and implements the desired functionality. Since the CI runs through, I recommend merging.

@github-actions
Copy link
Contributor

Thank you for the PR!

@JuanPedroGHM
Copy link
Member Author

run bench

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

1 similar comment
@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Thanks a lot @JuanPedroGHM , this is really helpful.

One general comment: it isn't obvious to me how to view the benchmark results once the Action has run.

Otherwise I only have tiny changes requests and am looking forward to merge. Thanks a lot!

uses: actions/checkout@v3
- name: Setup MPI
uses: mpi4py/setup-mpi@v1
- name: Use Python 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we take 3.9 or 3.10?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change both of them to python 3.10

- name: Use Python 3.8
uses: actions/setup-python@v4
with:
python-version: 3.8 # Perun only supports 3.8 and ahead
Copy link
Contributor

Choose a reason for hiding this comment

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

3.9 or 3.10?

Copy link
Member Author

Choose a reason for hiding this comment

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

About the link to the benchmarks, I will add a new badge to the README that points to the plots.

jobs:
benchmark-pr:
name: Benchmark PR
if: contains(github.event.pull_request.labels.*.name, 'PR talk')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a dedicated label, "benchmark PR" or something? Would make it more intuitive esp. for future contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

…pointing to the benchmarks, changed trigger tag
@github-actions
Copy link
Contributor

Thank you for the PR!

1 similar comment
@github-actions
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks a lot @JuanPedroGHM !

@ClaudiaComito ClaudiaComito added this to the 1.3.0 milestone Apr 27, 2023
@ClaudiaComito ClaudiaComito merged commit 02d2492 into main Apr 27, 2023
@ClaudiaComito ClaudiaComito deleted the features/continuous-benchmarking branch April 27, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants