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

Add performance benchmarks #748

Merged
merged 25 commits into from
Jan 11, 2023

Conversation

duckontheweb
Copy link
Contributor

@duckontheweb duckontheweb commented Feb 10, 2022

Related Issue(s):

Description:

Uses the airspeed velocity (asv) library to run performance benchmarks. This configuration generally follows the strategy used by xarray (as documented here), with some notable exceptions:

  • Increase the number of rounds to 4 (from default of 2) and the number of repetitions to between 10 & 50 (from default of 5). This seemed to eliminate a lot of the false positives with regard to performance changes. These values are set in the Bench base class and can be changed for a particular benchmark.
  • Use the --interleave-rounds option (also seems to help with eliminating false positives)
  • Run benchmarks on all supported Python versions (we may want to change this later as the benchmark suite grows and takes longer to run).

This is a work-in-progress, but I want to get feedback on the approach before fleshing out the rest.

Still to do:

  • Add timing benchmarks for extension implementations (maybe not all, but at least a few)
  • Add timing and peak memory benchmarks for reading and walking a catalog
  • Add timing benchmark for writing large catalogs to disk (e.g. on the order of thousands of items)
  • Come up with a strategy for measuring possible performance improvements from implementing async requests. We will probably see the most impact from this when making network requests rather than local file reads, but we don't want our benchmarks dependent upon changing network latency.
  • Add documentation

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

@duckontheweb
Copy link
Contributor Author

@TomAugspurger Would be great to get your feedback as well.

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

Base: 94.43% // Head: 94.43% // No change to project coverage 👍

Coverage data is based on head (5b39551) compared to base (c45dd20).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #748   +/-   ##
=======================================
  Coverage   94.43%   94.43%           
=======================================
  Files          83       83           
  Lines       12056    12056           
  Branches     1143     1143           
=======================================
  Hits        11385    11385           
  Misses        492      492           
  Partials      179      179           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@duckontheweb duckontheweb marked this pull request as draft February 10, 2022 18:53
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

LGTM. Did you have a running list of other scenarios you'd like to benchmark? One or two that spring to mind are:

  • Mutating the entire tree (e.g. setting the root href and updating all of the links/self hrefs)
  • Summary/extent computation

@duckontheweb duckontheweb mentioned this pull request Feb 14, 2022
9 tasks
@duckontheweb
Copy link
Contributor Author

duckontheweb commented Feb 15, 2022

LGTM. Did you have a running list of other scenarios you'd like to benchmark? One or two that spring to mind are:

  • Mutating the entire tree (e.g. setting the root href and updating all of the links/self hrefs)
  • Summary/extent computation

Thanks @gadomski, I have a bit of a list started in #729. I like both of these suggestions, I'll add them to the PR.

@gadomski gadomski self-assigned this Jan 9, 2023
I'm of the opinion that we _shouldn't_ run benchmarks on Github runners, so I'm
removing this workflow.
This lets simple command like `asv dev` work out of the box.
I'm not really sure how useful this is, but it was asked for so at least we have
something.
This doesn't run benchmarks, but just checks to make sure they build.
@gadomski
Copy link
Member

gadomski commented Jan 9, 2023

I picked this PR up and, in the interest of not letting perfect be the enemy of good, implemented most of @duckontheweb's suggestions:

  • I added benchmarks for reading, walking, and writing large catalogs, including peak memory usage for walking a large catalog
  • I added a single extension benchmark, for the projection extension. I'm not quite sure what we want to be benchmarking there, so I kept it minimal.
  • I refactored the location of things to make usage a bit simpler (e.g. I want asv dev to work out-of-the-box)
  • I removed CI benchmarking. To paraphrase @TomAugspurger from https://tomaugspurger.github.io/posts/performance-regressions, "This rules out running the benchmarks on ... CI ... even if we could finish it in time, we couldn’t really trust the results." It's my personal opinion that benchmarking on CI is so finicky and untrustworthy that its not worth the complexity. In liu of a dedicated benchmark machine, IMO it's better to run benchmarks locally when the situation calls for it. At least we have a framework for benchmarking now, which we could always expand in the future to a dedicated machine if its needed.
  • Docs.
  • Simple check in CI to make sure benchmarks run w/o failure -- doesn't do any reporting.

I didn't implement:

  • Any sort of async strategy. I think that's best tackled as part of the async improvements themselves (Add async I/O methods [WIP] #749) -- at least, I don't think they should hold up getting some sort of benchmarking into the repo.
  • Extensive extension benchmarking. I'm not sure where the pain points are right now, and I don't want broken benchmarks to get in the way of solutions to Handling STAC extension version upgrades #448.

cc @duckontheweb would love your review if you have the bandwidth, I can't request b/c it's your PR.

IMO this PR should be squash-merged since it's one coherent set of changes w/ some fixup commits -- I can't rebase b/c it's on @duckontheweb's fork.

@gadomski gadomski marked this pull request as ready for review January 9, 2023 23:32
@gadomski gadomski requested review from pjhartzell and removed request for lossyrob January 9, 2023 23:32
@gadomski gadomski changed the title Add performance benchmarks [RFC] Add performance benchmarks Jan 9, 2023
Copy link
Collaborator

@pjhartzell pjhartzell left a comment

Choose a reason for hiding this comment

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

Looking good. This is a nice addition.

docs/contributing.rst Outdated Show resolved Hide resolved
benchmarks/import_pystac.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pjhartzell pjhartzell left a comment

Choose a reason for hiding this comment

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

Looks good.

@gadomski gadomski merged commit 2aaa162 into stac-utils:main Jan 11, 2023
@gadomski gadomski added this to the 1.7 milestone Jan 18, 2023
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.

Establish performance testing framework and benchmarks
4 participants