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

Apply prewarm when benchmarking #362

Merged
merged 21 commits into from
Sep 15, 2022
Merged

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Sep 14, 2022

Wanted to make a couple of changes to improve benchmarking cron. As part of this, realized that the use cases for the test cron and benchmarking cron are diverging so I factored them out into separate code paths for easier reading.

Changes:

  • prewarm phase to rule out any further/late plugin installation
  • separate SKIP config, skip static-website templates for now
  • use dev versions of Pulumi CLI instead of release versions; faster feedback
    • had to roll this back since with bors changes in p/p we do not have a usable "dev" pointer at the moment
  • remove parallelism from benchmarks to get better numbers
  • higher duration of aws token session to compensate for longer runtimes and avoid failures to upload data

Also I've gotten to the bottom of PULUMI_TEMPLATE_LOCATION use I think - if unset, this tests released templates, but if set to PWD it uses locally modified copies. This is setup just fine on CI but is confusing locally when trying to test out template changes on my machine, by default the tests just ignore those changes unless I remember to set this env var. I've setup convenient make targets and documented this detail, but there's no changes to how CI runs things with respect to this env var.

@t0yv0
Copy link
Member Author

t0yv0 commented Sep 14, 2022

@t0yv0
Copy link
Member Author

t0yv0 commented Sep 14, 2022

Prewarm + no parallelism makes it take a lot more time but I feel it is worth it. The numbers popping out are smaller again, and hopefully the variance will be smaller also, making them much more useful to us.

@t0yv0 t0yv0 marked this pull request as ready for review September 14, 2022 20:17
Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

Nice! I left a few comments at your discretion.

.github/workflows/performance_metrics_cron.yml Outdated Show resolved Hide resolved
.github/workflows/performance_metrics_cron.yml Outdated Show resolved Hide resolved
.github/workflows/performance_metrics_cron.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@t0yv0
Copy link
Member Author

t0yv0 commented Sep 15, 2022

@lblackstone adding in case you can represent providers team here - editing shared code, wanted to make sure it's ok.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

LGTM

@t0yv0 t0yv0 merged commit 7b74c4b into master Sep 15, 2022
@pulumi-bot pulumi-bot deleted the t0yv0/apply-prewarm-when-benchmarking branch September 15, 2022 21:17
@t0yv0 t0yv0 added this to the 0.78 milestone Sep 15, 2022
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.

None yet

3 participants