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

ASV Benchmarking Pull Request Workflow #831

Merged
merged 2 commits into from
Jul 2, 2024
Merged

ASV Benchmarking Pull Request Workflow #831

merged 2 commits into from
Jul 2, 2024

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Jul 2, 2024

Overview

Introduces a Github Actions Workflow for running ASV Benchmarks on Pull Requests, getting compared to the main branch.

The workflow is triggered using the run-benchmark label.

image

By default, the workflow is skipped unless it is queued using the label above.

image

The workflow can be re-queued by removing and adding the label.

image

Once the workflow is complete, the results are posted (or updated) by a Github Bot.

New benchmarks, including those that test new functionality, can be run using this bot as well. They will fail for older versions that raise an error, but that is expected.

Example screenshot of the output:

image

@philipc2
Copy link
Member Author

philipc2 commented Jul 2, 2024

An example of this workflow in action can be seen in this dummy PR on my fork.

philipc2#2

@philipc2 philipc2 added the CI Continuous Integration label Jul 2, 2024
@philipc2 philipc2 self-assigned this Jul 2, 2024
@philipc2 philipc2 linked an issue Jul 2, 2024 that may be closed by this pull request
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

ASV Benchmarking

Benchmark Comparison Results

Benchmarks that have improved:

Change Before [3b27d3f] After [7bd5fec] Ratio Benchmark (Parameter)
- 388M 301M 0.78 mpas_ocean.Integrate.peakmem_integrate('480km')

Benchmarks that have stayed the same:

Change Before [3b27d3f] After [7bd5fec] Ratio Benchmark (Parameter)
240M 240M 1 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
300M 300M 1 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
318M 305M 0.96 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
316M 301M 0.95 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
13.9±0.02s 14.0±0.09s 1.01 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
2.06±0.01s 2.09±0s 1.01 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
19.2±0.02s 19.8±0.01s 1.03 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
72.2±0.4ms 74.1±2ms 1.03 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
1.66±0.02s 1.66±0.01s 1 import.Imports.timeraw_import_uxarray
5.70±0.05μs 5.72±0.05μs 1 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('120km')
5.24±0.01μs 5.31±0.02μs 1.01 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('480km')
367M 369M 1.01 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('120km', False)
346M 349M 1.01 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('120km', True)
320M 320M 1 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('480km', False)
319M 319M 1 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('480km', True)
1.20±0s 1.22±0.01s 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', False)
55.1±0.5ms 55.5±0.4ms 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', True)
93.9±0.4ms 93.6±0.3ms 1 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', False)
5.33±0.07ms 5.20±0.1ms 0.98 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', True)
253M 253M 1 mpas_ocean.Gradient.peakmem_gradient('120km')
233M 233M 1 mpas_ocean.Gradient.peakmem_gradient('480km')
2.65±0.03ms 2.66±0.01ms 1.01 mpas_ocean.Gradient.time_gradient('120km')
282±0.5μs 282±0.9μs 1 mpas_ocean.Gradient.time_gradient('480km')
318M 320M 1.01 mpas_ocean.Integrate.peakmem_integrate('120km')
176±1ms 174±2ms 0.99 mpas_ocean.Integrate.time_integrate('120km')
11.8±0.1ms 11.9±0.1ms 1.01 mpas_ocean.Integrate.time_integrate('480km')
229M 231M 1.01 quad_hexagon.QuadHexagon.peakmem_open_dataset
229M 229M 1 quad_hexagon.QuadHexagon.peakmem_open_grid
6.73±0.1ms 6.63±0.06ms 0.98 quad_hexagon.QuadHexagon.time_open_dataset
5.75±0.05ms 5.70±0.02ms 0.99 quad_hexagon.QuadHexagon.time_open_grid

@philipc2 philipc2 marked this pull request as ready for review July 2, 2024 01:02
Copy link
Contributor

@rajeeja rajeeja left a comment

Choose a reason for hiding this comment

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

This is excellent and very timely for our project.
We can tackle some of my comments in another future PR.

The table units of before and after and the function calls look a little verbose.

ci/environment.yml Show resolved Hide resolved
benchmarks/quad_hexagon.py Show resolved Hide resolved
benchmarks/quad_hexagon.py Show resolved Hide resolved
.github/workflows/asv-benchmarking-pr.yml Show resolved Hide resolved
@philipc2
Copy link
Member Author

philipc2 commented Jul 2, 2024

This is excellent and very timely for our project. We can tackle some of my comments in another future PR.

The table units of before and after and the function calls look a little verbose.

Thanks for the review! Yeah, the table can definitely be trimmed a bit to look cleaner. I didn't do any processing of the data, simply returned the result of the asv compare function call.

I'll definitely look into at least removing the Change column, since that's not being populated at all and is just taking up space.

Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

This is incredibly useful! Despite some issues (e.g. the Change column), I am fine with getting this merged and fixing those issues in a separate PR since this will not break any functionality and instead will be usable for other PRs immediately after the merge.

@philipc2 philipc2 merged commit 07813fa into main Jul 2, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration run-benchmark Run ASV benchmark workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASV Benchmark PR Workflow
3 participants