Add workflow to test perf using sightglass #3740
Conversation
|
@fitzgen @cfallin @alexcrichton @abrown This does not work now in this particular PR because self hosted runner support is no enabled in this repository but here is an example of what this would look like: jlb6740#2. At that link pay particular attention to the comment and table that only shows percent change, the table that shows the mean column will be removed as it is only desired to show percent change. More notes:
|
jlb6740
force-pushed
the
add-sightglass-perf-actions
branch
2 times, most recently
from
0a47221 to
ebd998e
Compare
jlb6740
force-pushed
the
add-sightglass-perf-actions
branch
from
ebd998e to
c9b0cbe
Compare
| # Workflow runs when manually triggered using the UI or API. | ||
| on: | ||
| push: | ||
| pull_request: |
There was a problem hiding this comment.
I think we'll want to confirm the security story here before merging this. Unfortunately hooking up hosted runners to public repos is explicitly recommended against in the github actions documentation because it becomes pretty easy to run arbitrary code on these hosted runners from external contributors via PRs. I think there's various measures we could put in place to mitigate that but we'll want to make sure that's all in order before having this all hooked up.
There was a problem hiding this comment.
I think there is some mechanism for generating/confirming with tokens that may be what we want. I don't believe I use that here as I was just following the github doc for a basic setup but it is something I'll look into. Also, since you highlighted the on: push/pull_request, note I want to change this to be on comment .. where the trigger occurs on a comment with the string /bench_x64 or something like that.
There was a problem hiding this comment.
One thing I'd be worried about is that once we have a custom runner hooked up any contributor can make a PR with a new workflow file to run on all PRs which uses the custom runner which I think means we basically don't have any ability to limit what runs on the runner unless we can hard configure it to ignore PRs entirely (or something like that)
There was a problem hiding this comment.
This was discussed today and I think the solution is to just only allow to have the privileges to run. Anyone else can run what this is running offline before committing. I will look into a way to have this be triggered via a comment (which I already have but need to work out a wrinkle) and then look to see if we can discover the author of that comment to allow running of the code based on that comment author.
There was a problem hiding this comment.
FWIW I'm mostly parroting this which says:
We recommend that you only use self-hosted runners with private repositories
and this
As a result, self-hosted runners should almost never be used for public repositories on GitHub
I don't know why the "almost" is there, though.
|
|
||
| # Env variables | ||
| env: | ||
| SG_COMMIT: 649509c |
There was a problem hiding this comment.
Is it possible to use the main branch of sightglass? (unsure if this is largely for testing or whether it's intended that we periodically update this)
There was a problem hiding this comment.
This is a ref from the main branch of sightglas .. it's actually HEAD as of today. However I wanted to talk about versioning with sightglass so as to be more transparent about what we are using to run tests.
| with: | ||
| ref: 'main' | ||
| submodules: true | ||
| path: wasmtime_main |
There was a problem hiding this comment.
When a push to main happens I think this'll double-checkout main and build it twice?
Ideally though it'd be awesome if we could do just one build as part of this PR and use stored data for previous runs perhaps for comparison. We should be guaranteed that all commits on main have performance information so comparing a new merge to main or a PR is ideally just about picking the revision to compare against.
There was a problem hiding this comment.
To be honest, I think there is some caching that is taking place via this github code that lives on the server. I do though the blast the sightglass directory and rebuild that. I don't blast the wasmtime directories but at some point during developing this it seemed those directories were removed but then very quickly rebuilt by the environment, implying to me that something was cached.
This iteration doesn't actually test against much of anything (that will be critiqued for improvement), but as a result this entire process from start to finish only takes about 4-1/2 minutes, so even if everything is rebuilt it currently really doesn't take that long. Also like I mentioned above (and suggested offline), I want this to only run on demand not after every PR is submitted or updated. For this patch, I wanted to have something that was for sure going to work every time and then get fancy with the caching and other efficiency improvements on a future iteration. If there is something obvious to change now though, lets do it.
There was a problem hiding this comment.
Ah ok if it's fast enough then seems reasonable to at least start with this and we can alter later if it ever becomes necessary
| - name: Setup Rust Toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable |
There was a problem hiding this comment.
Personally I'd prefer to keep the usage of external actions to a minimum for the wasmtime repo, so here if we can arrange for rustup to be installed on the hosted runner this I think could be rustup update stable && rustup default stable
| id: findPr | ||
| with: | ||
| # Can be "open", "closed", or "all". Defaults to "open". | ||
| state: open |
There was a problem hiding this comment.
I think we might be able to detect this with various env vars set on github actions for prs?
There was a problem hiding this comment.
Although if not my main concern about using third-party actions is the security implications. AFAIK we're largely giving all these actions read/write access to the wasmtime repo so a push to this jwalton/gh-find-current-pr repo could end up compromising us.
One way to mitigate this perhaps would be to run this workflow in a separate repository that is triggered from this repository (or something like that). That way if our other repository gets messed up it's not the end of the world.
There was a problem hiding this comment.
I think we can get away with not using this findPr or other third-party actions which I'll try to do. That said, when you say "AFAIK we're largely giving all these actions read/write access to the wasmtime repo so a push to this jwalton/gh-find-current-pr repo could end up compromising us." do you mean read/write access to just the downloaded wasmtime repository that is housed on the remote server or are you thinking access to the wasmtime repository as it lives in github?
There was a problem hiding this comment.
I believe that workflows generally get read/write tokens to the bytecodealliance/wasmtime repository that lives on github, so I'm under the impression that a malicious workflow could compromise big chunks of the source code and repo
|
Closing in favor of #4421 |
This is a workflow intended to run sightglass on a self-hosted runner.