search

lorenzwalthert / touchstone

Smart benchmarking of pull requests with statistical confidence
https://lorenzwalthert.github.io/touchstone
Other
53 stars 7 forks source link

Use the head branch's touchstone script. #134

Closed plietar closed 4 months ago

plietar commented 4 months ago

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.

plietar commented 4 months ago

cc @assignUser who made the #112 PR, might have had a rationale for changing this that I am missing

plietar commented 4 months ago

Thanks for the feedback.

112 replaced a git branch with a git switch -c. The former only creates a branch, whereas the later creates a branch and switches the worktree to it. This changes the state of the current working directory, and influences which version of the script gets read.

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.

Benchmark: small_population
  ℹ Running 8 iterations of benchmark: small_population.
  ✔ Temporarily checked out branch "master".
  ✔ Ran 1 iteration of branch "master".
  ✔ Switched back to branch "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 cb-test (ie. the PR branch) instead:

  ✔ Temporarily checked out branch "master".
  ✔ Ran 1 iteration of branch "master".
  ✔ Switched back to branch "cb-test".

In both cases, the branch that gets switched back to is whatever was checked out when the script started running.

I also added the -f flag because it seemed the code was trying to overwrite the branch if it exists, by deleting it (git branch -D) and creating it again (as part of git switch -c), which is exactly what git branch -f does. Sorry that was a bit of an orthogonal change.

For now I'm just playing around with touchstone, but the goal is to use it in mrc-ide/malariasimulation and mrc-ide/individual.

assignUser commented 4 months ago

Wow thanks for the detailed response! :heart:

The former only creates a branch, whereas the later creates a branch and switches the worktree to it

Ah! I was so focused on the state of the branch that I totally missed that :facepalm: 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?

lorenzwalthert commented 4 months ago

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.