You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi everyone! Not sure whether this is the right place, but happy to re-open elsewhere if that makes sense:
I’m really digging the benchmark CI functionality that we now have – great work!
Usually, I like to check the benchmarks on something before opening a PR – if the outcome is problematic, I can adjust or rework the changes as needed before it makes sense for the actual review process to start.
For the regular CI or CITGM, that’s pretty easy because we can set the github repository + the git ref name for node-test-commit freely, and it would be awesome if the benchmark CI supported a similar functionality.
I realize that we should run benchmarks locally anyway; but the benchmark CI has the advantage of being fast compared to many developer’s machines, and a dedicated resource (and not battery-draining, for laptop-using devs).
If I can help I’m happy to, but I imagine this is mostly a Jenkins script thing?
So you probably want to raise an issue or PR in that repo and see what people think. I'm happy to change the configuration in Jenkins if the run.sh script gets changed.
Agree with the suggestion to open issue/pr in the benchmarking WG as that is where @gareth-ellis (he put it together) will most likely see it an comment.
@sam-github See nodejs/benchmarking#195 – basically, nobody with some understanding of the Jenkins config seems interested in this. I guess now that I’m in the build WG I could try to figure out how to do that, but I’d basically still need somebody to show me how to, I think.
It doesn't appear to error if no TARGET is set (I haven't defaulted TARGET to any value) -- perhaps the script should exit with a non-zero code if a mandatory option isn't set?
I couldn't seem to get it to work with a BASE branch other than master (probably whatever the default branch is). Tried v13.x and refs/heads/v13.x. I don't know if this is because I'm incorrectly specifying the gitref or if it's something to do with the way the repository is cloned in the script.
(edit: trying remotes/origin/v13.xv13.x works but only for TARGET and not for BASE)
I think the above two issues are with the script being run, so would need updates to nodejs/benchmarking#195. Please post in this issue if further updates are required to the job.
Once nodejs/benchmarking#195 is merged the job will need to be edited so that it picks up the benchmarking script from the nodejs/benchmarking repository instead of the fork.
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
Activity
addaleax commentedon Jan 1, 2018
/cc @mhdawson
gibfahn commentedon Jan 2, 2018
Which job are you wanting to run?
addaleax commentedon Jan 2, 2018
@gibfahn Basically, I’d like to run https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/ except with the
GITHUB_ORG,REPO_NAMEandGIT_REMOTE_REFparameters that we have for e.g.node-test-commit, if that’s possible at all?gibfahn commentedon Jan 2, 2018
Looking at the job, it's actually running: https://github.com/nodejs/benchmarking/blob/core-benchmark/experimental/benchmarks/community-benchmark/run.sh
So you probably want to raise an issue or PR in that repo and see what people think. I'm happy to change the configuration in Jenkins if the run.sh script gets changed.
mhdawson commentedon Jan 3, 2018
Agree with the suggestion to open issue/pr in the benchmarking WG as that is where @gareth-ellis (he put it together) will most likely see it an comment.
Update community-benchmark/run.sh to allow generic github repos/branches
sam-github commentedon Jan 28, 2020
@addaleax are you still working on this?
addaleax commentedon Jan 28, 2020
@sam-github See nodejs/benchmarking#195 – basically, nobody with some understanding of the Jenkins config seems interested in this. I guess now that I’m in the build WG I could try to figure out how to do that, but I’d basically still need somebody to show me how to, I think.
richardlau commentedon Feb 1, 2020
@addaleax I've created https://ci.nodejs.org/job/benchmark-node-micro-benchmarks-compare/ which is a copy of https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/ with the parameters changed to match the "Use case 2" of the script in nodejs/benchmarking#195 (and the job for the moment is cloning https://github.com/addaleax/benchmarking/tree/generic-github (the branch for that PR)).
There are a few issues:
TARGETis set (I haven't defaultedTARGETto any value) -- perhaps the script should exit with a non-zero code if a mandatory option isn't set?BASEbranch other thanmaster(probably whatever the default branch is). Triedv13.xandrefs/heads/v13.x. I don't know if this is because I'm incorrectly specifying the gitref or if it's something to do with the way the repository is cloned in the script.(edit:
tryingremotes/origin/v13.xv13.xworks but only forTARGETand not forBASE)richardlau commentedon Feb 1, 2020
New job appears usable in its current state as long as you are comparing against
master(see the two issues in the above comment).e.g. https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks-compare/14/ is a run that could have been made for nodejs/node#31605 prior to opening a PR (which was the original requirement).
I think the above two issues are with the script being run, so would need updates to nodejs/benchmarking#195. Please post in this issue if further updates are required to the job.
Once nodejs/benchmarking#195 is merged the job will need to be edited so that it picks up the benchmarking script from the nodejs/benchmarking repository instead of the fork.
addaleax commentedon Feb 10, 2020
@richardlau Thanks for the pings! I’ll try to fix the issues you mentioned. :)
mscdex commentedon Feb 11, 2020
I've also noticed that using commit hashes for the
BASEandTARGETcauses the process to fail with:github-actions commentedon Dec 8, 2020
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
anonrig commentedon Mar 21, 2023
Can we reopen this issue, or remove the existing job from Jenkins (which is not completed I assume)? @nodejs/build