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

Enable Nyrkiö on PRs and clickbench #959

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

henrikingo
Copy link
Contributor

@henrikingo henrikingo commented Feb 10, 2025

  • Rename push_only.yml workflow to rust_perf.yml
  • Move the 'bench' task from rust.yml to rust_perf.yml
  • More Nyrkio configuration options exposed:
    Team support (everyone in gh/tursodatabase can access Nyrkiö)
    Public results
    Alerting options: Comment on PR, if change detected, don't fail task
  • Add Nyrkio also to analyse results from the clickbench task

- uses: actions/checkout@v3
- name: Bench
run: cargo bench

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You added quite a lot of benchmarks the past 2 weeks and this now takes 7 minutes or more, so I suggest to separate.

No need for Nyrkiö to do that. Just a suggestion in general.

@henrikingo

This comment was marked as resolved.

@henrikingo
Copy link
Contributor Author

TL;DR: Suggest to merge this version. Longer term can add support for outside contributors on the nyrkio.com side.

Ok I did some reading and there are 3 options:

  • It is by design that I don't get access to any of your secrets when I'm not member of the tursodatabase team. More precisely, I think it's when I don't have write access to the repo. The first option is simply to accept this and merge this PR as it is now. For outside contributors this will exit immediately, because the Nyrkiö token is missing from the input. But it will still exit without error, so the PR looks green. (never-fail: true). If you have a PR that is expected or suspected to impact performance, you can always have a developer who has commit permission resubmit the PR and it should then run the analysis through Nyrkiö.

  • It is possible to run this task in the context of the repo owner rather than the PR author. This happens if we put 'on: pull_request_target' into the top of the file. This is clearly unsafe. A variation of this would be to supply the Nyrkiö token somehow outside the secrets facility. I considered this but it too is undesirable. The way it works now anyone could login to nyrkio.com impersonating Pekka....

  • I thought about it and I think it is possible for us to support / align with the github permissions model, in cases where the performance results are public. Specifically, we could change out pulls/ API so that it needs the PR author to have a Nyrkiö account, and we store the PR perf results separately in each contributors account. For the comparison we then append the PR test result to the publicly available historical results of the main branch.

  • The last option would be to not run Nyrkiö at all for PRs, just post merge. This is an ok option too, if you don't like the first option in above list..

Set pvalue and threshold from workflow
- Rename push_only.yml workflow to rust_perf.yml
- Move the 'bench' task from rust.yml to rust_perf.yml
- More Nyrkio configuration options exposed:
  Team support (everyone in gh/tursodatabase can access Nyrkiö)
  Public results
  Alerting options: Comment on PR, if change detected, don't fail task
This allows PRs to succeed regardless of who submitted them. Feels
friendlier that way.
- Rename push_only.yml workflow to rust_perf.yml
- Move the 'bench' task from rust.yml to rust_perf.yml
- More Nyrkio configuration options exposed:
  Team support (everyone in gh/tursodatabase can access Nyrkiö)
  Public results
  Alerting options: Comment on PR, if change detected, don't fail task
@henrikingo henrikingo changed the title Enable Nyrkiö also on PRs. (Non-blocking mode.) Enable Nyrkiö on PRs and/or clickbench Feb 18, 2025
@henrikingo henrikingo changed the title Enable Nyrkiö on PRs and/or clickbench Enable Nyrkiö on PRs and clickbench Feb 18, 2025
@henrikingo
Copy link
Contributor Author

Sorry about the mess above. I accidentally pushed some different branch when I started to work on the clickbennch integration. But now everything should be good.

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.

1 participant