-
Notifications
You must be signed in to change notification settings - Fork 7
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
Use the head branch's touchstone script. #134
Conversation
When setting up branches, we were switching from the head to the base branch. This implies that the benchmarking script being used is that of the base branch. I believe it makes more sense to use the head branch's script, as when one adds a new benchmark to an existing script, you would want to see to results of it without having to merge that PR first. This actually used to be the behaviour but was changed as part of lorenzwalthert#112. I cannot see any discussion about this change so I suspect it may have been unintentional.
cc @assignUser who made the #112 PR, might have had a rationale for changing this that I am missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something but doesn't your current change just combine git branch -D
and git switch -c
into git branch -f
? The actual ref/remote that is checked out stays the same?
The ref is also the same (except taking fork PRs into account) as before #112 afaik?
- git branch $GITHUB_BASE_REF remotes/origin/$GITHUB_BASE_REF
+ git switch -c ${{ env.GITHUB_BASE_REF }} $REMOTE/${{ env.GITHUB_BASE_REF }}
On an unrelated note: Thanks for your contributions! Where are you using {touchstone} if I may ask?
Thanks for the feedback. #112 replaced a This PR effectively undoes that change to make sure we stay on the original branch. Now the block of code only creates a branch, and does not change the worktree. You can see the behaviour in some of my test runs: https://github.com/plietar/malariasimulation/actions/runs/9111758056/job/25049559033 uses lorenzwalthert/touchstone@v1. After each iteration of the benchmark it switches back to master.
After moving my workflow to point to my fork of touchstone (eg. https://github.com/plietar/malariasimulation/actions/runs/9113745723/job/25056027590), after each iteration we switch back to is
In both cases, the branch that gets switched back to is whatever was checked out when the script started running. I also added the For now I'm just playing around with touchstone, but the goal is to use it in mrc-ide/malariasimulation and mrc-ide/individual. |
Wow thanks for the detailed response! ❤️
Ah! I was so focused on the state of the branch that I totally missed that 🤦 You are right, in that case I think it makes sense to return the previous behavior. Being able to add new benchmarks certainly is a feature you'd want. @lorenzwalthert I assume this was also originally your intention? |
Thanks guys for the detailed discussion. I approve of this change. Also thanks for your contribution @plietar. I don’t have the bandwidth to work on this repo anymore myself, but contributions are welcome. |
When setting up branches, we were switching from the head to the base branch. This implies that the benchmarking script being used is that of the base branch.
I believe it makes more sense to use the head branch's script, as when one adds a new benchmark to an existing script, you would want to see to results of it without having to merge that PR first.
This actually used to be the behaviour but was changed as part of #112. I cannot see any discussion about this change so I suspect it may have been unintentional.