Skip to content

Annotate merge-base comparisons; improve report links#226

Open
IanButterworth wants to merge 1 commit into
masterfrom
ib/merge_base
Open

Annotate merge-base comparisons; improve report links#226
IanButterworth wants to merge 1 commit into
masterfrom
ib/merge_base

Conversation

@IanButterworth

@IanButterworth IanButterworth commented Apr 19, 2026

Copy link
Copy Markdown
Member

Comments in the code indicated we use the merge base, but it doesn't actually seem we do.
Actually, it's just a footgun of using the vs=:master form when you just want a PR diff. Adds user guidnace.

Developed with claude:

Updated


Add transparency around merge-base comparisons (introduced in #134):
reports and reply comments now indicate when a merge-base was used and
show which branch it was computed from. When vs explicitly references
the PR's base branch (e.g. vs=":master" on a PR targeting master),
add a hint suggesting to omit vs to get the merge-base comparison.

Also:

  • Show SHA range in Comparison Diff/Range links instead of generic "link"
  • Remove unused prnumber parameter from build_julia!
  • Remove stale comments about merge commit building (never implemented)
  • Simplify build_benchmarksjulia! call site

@maleadt

maleadt commented Apr 19, 2026

Copy link
Copy Markdown
Member

We are using the merge base, but only when vs is unspecified: #134
I'm don't think we want the proposed behavior. When specifying vs, to me that indicates you really want to compare against that branch or commit, and not against some derivative point in history.

@IanButterworth

Copy link
Copy Markdown
Member Author

Ah ok. I remember that now. I think the current common runbenchmarks(ALL, vs=":master") has become a bit of a footgun. The report/comment should probably be clearer what's happening/guide the user.

@IanButterworth

Copy link
Copy Markdown
Member Author

Updated to just improve user guidance

@IanButterworth IanButterworth changed the title Use merge-base for fair PR comparisons; improve report links Annotate merge-base comparisons; improve report links Apr 19, 2026
Add transparency around merge-base comparisons (introduced in #134):
reports and reply comments now indicate when a merge-base was used and
show which branch it was computed from. When `vs` explicitly references
the PR's base branch (e.g. `vs=":master"` on a PR targeting master),
add a hint suggesting to omit `vs` to get the merge-base comparison.

Also:
- Show SHA range in Comparison Diff/Range links instead of generic "link"
- Remove unused `prnumber` parameter from `build_julia!`
- Remove stale comments about merge commit building (never implemented)
- Simplify `build_benchmarksjulia!` call site

Co-Authored-By: Claude <noreply@anthropic.com>
@maleadt

maleadt commented Apr 19, 2026

Copy link
Copy Markdown
Member

The recommendation should just be to do runbenchmarks(ALL) or runtests(), both of which use the merge base, while allowing people (mostly @KristofferC) to compare against specific branches using vs.

@IanButterworth

Copy link
Copy Markdown
Member Author

Yeah, I'm just seeing a lot of use of vs=:master still https://github.com/search?q=repo%3AJuliaLang%2Fjulia+%22runbenchmarks%22&type=pullrequests&s=created&o=desc

This just tries to make it clearer.

@maleadt

maleadt commented Apr 19, 2026

Copy link
Copy Markdown
Member

With runbenchmarks that was also what people had to do until recently (presumably that's fixed now) because the benchmark worker hadn't been updated in a long time, and didn't include the above PR.

@IanButterworth

Copy link
Copy Markdown
Member Author

Ah I see.

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.

2 participants