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

Workflows to benchmark using ASV #250

Merged
merged 13 commits into from
Aug 9, 2023
Merged

Conversation

camposandro
Copy link
Contributor

@camposandro camposandro commented Aug 4, 2023

Creates GitHub workflows to benchmark methods using airspeed velocity (ASV).

Tested for benchmarking-asv mock project (live dashboard).

asv-main

Notes:

  • It runs benchmarks for all the commit hashes on main for which there are no benchmark results. The first run will take a while because it will process every commit on the repository to the present day, but subsequent runs are faster.
  • A github-actions[bot] pushes the benchmark results to the main branch. The workflows run consecutively to avoid any conflicts between jobs attempting to submit results simultaneously. A workflow is queued up until the previous workflow running on main is finished.
  • ASV uses the most recent benchmarking suites to compute results for the range of commits in question. Any direct change to these suites with the intent to affect the runtime or memory usage produces no change of behavior.

asv-pr

  • Runs benchmarks for pull requests.
  • Compares the performance of the main branch with the performance of the merge with the new changes.
  • Publishes a comment on the pull request with this assessment.

Example of a comment:

Before After Ratio Method
[fcd6c976] [bc939276]
failed 304±2ms n/a benchmarks.TimeSuite.time_iterkeys
2.43±0.05μs 205±0.7ms 84400.48 benchmarks.TimeSuite.time_keys
9.67±0.03μs 505±1ms 52177.14 benchmarks.TimeSuite.time_range
failed 1.01±0s n/a benchmarks.TimeSuite.time_xrange

Click here to view all benchmarks.

asv-nightly

  • Runs benchmarks every morning at 6:45.
  • Compares the performance of the main branch with the one from the previous day.

Notes:

  • This workflow uses the repository cache to store the results for each nightly run. GitHub will remove any cache entries that have not been accessed in over 7 days so we should not worry about them compounding over time.

Checklist

  • This PR is meant for the lincc-frameworks/python-project-template repo and not a downstream one instead.
  • This change is linked to an open issue
  • This change includes integration testing, or is small enough to be covered by existing tests

Relates to #79.

@camposandro camposandro self-assigned this Aug 4, 2023
@camposandro camposandro requested a review from drewoldag August 4, 2023 18:23
@drewoldag
Copy link
Collaborator

This is looking pretty interesting - I left a could questions in the code, but I have a few others that I'm curious to get your take on:

Is there any way to use the unit tests as the benchmarked code? Currently what is written is pretty explicit, I'm curious if we could use some introspection to list all the methods in the pytest classes and run those dynamically with the .runtime_computation and .memory_computation? Or perhaps only run those classes that are tagged with a specific pytests.mark.<something> annotation? Please forgive me if you've already looked deeply into this.

Documentation will be key here - including access perhaps to some examples so that people can see what they are getting. It doesn't necessarily have to be part of this PR, but we should be sure to include a reasonable amount of documentation around what ASV does. We have a section, https://lincc-ppt.readthedocs.io/en/latest/practices/overview.html, that describes many of the utilities that are included with the PPT, and attempts to describe, at a very high level, how to use them. This work should definitely be included there. Additionally, we list all the copier questions a user could encounter (https://lincc-ppt.readthedocs.io/en/latest/source/new_project.html#create-a-new-project-from-the-template) we should expand that to include any new questions introduced here.

Is there any additional setup required for users to publish the results to GH pages for a given project? i.e. do we need to tell them to click a button in the UI before they start?

Copy link
Collaborator

@drewoldag drewoldag left a comment

Choose a reason for hiding this comment

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

I've left a few questions in the code and in a comment here. I would love to get your take on them when you have a moment. I'm happy to set up some time to chat face to face too if that's easier :)

Copy link
Contributor

@delucchi-cmu delucchi-cmu left a comment

Choose a reason for hiding this comment

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

We should also update RTD with the new practice.

  • list of options and more help text in docs/source/new_project.rst
  • a new practices/benchmarking.rst with the how and why
  • any initial steps someone might need to undergo to make it work the first time

@camposandro
Copy link
Contributor Author

camposandro commented Aug 8, 2023

I've extracted the format script to its own PyPI package (lf-asv-formatter) and added relevant documentation to the RTD website! Could you please have a new look? I feel like I'm so deeply involved into this feature that I might be forgetting to write essential information. Thanks :)

@camposandro camposandro merged commit 275e077 into main Aug 9, 2023
@camposandro camposandro deleted the issue/79/add-asv-workflows branch November 3, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants