ocurrent / current-bench

Experimental benchmarking infrastructure using OCurrent pipelines
Apache License 2.0
33 stars 17 forks source link

Allow running benchmarks from a different repo #370

Closed punchagan closed 1 year ago

punchagan commented 2 years ago

This is a PR that corresponds to the changes in https://github.com/art-w/ocaml-benching/pull/2. This is still a WIP for the UI/frontend, but the backend plumbing should be in place.

ElectreAAS commented 1 year ago

With 26 files changed, this is a hard PR to review without context. Can you elaborate on what your goals were? I can guess that the addition of target_name and target_version is the major thing going on, but I don't understand what they really mean for us

punchagan commented 1 year ago

With 26 files changed, this is a hard PR to review without context. Can you elaborate on what your goals were?

Yes, apologies @ElectreAAS ! I just cleaned-up an old work-in-progress PR, and didn't realize that there's not enough context for a review. Thanks for flagging this! :)

The PR aims to allow having benchmarks in a different repository from the actual code that is being benchmarked. The original work was done to allow having the benchmarks for ocaml/ocaml run without making any changes to the ocaml/ocaml repository, including adding the GitHub application on the original/target repository.

With these changes, current-bench would be configured to watch the benchmark repository (art-w/benching for instance), and the make bench in that repository would clone the target repository and run the benchmarks. The current-bench code as it is, doesn't allow for this easily, since the current-bench requires commit hashes to be unique for a repository, and we'd like to be able to run the same benchmarks for different commits/versions of the target repository. The changes in this PR essentially allow for this, while also making some changes in the UI to link to the target repository instead of the benchmarks repository from the graphs, etc.

ElectreAAS commented 1 year ago

I just rebased on main for a simpler review. This was an automatic rebase so I assume nothing broke

punchagan commented 1 year ago

The current approach tries to support having the benchmarks in a completely different repository, and some configuration on current-bench without having to make any changes to the target repository. But, this doesn't support watching the target repository's PRs, etc., and the step of adding the GitHub application to the target repository would need to be done to support that. The changes in this PR were done specifically to support building benchmarks for ocaml/ocaml, and to allow for running the benchmarks on different versions of OCaml.

Currently, we plan to run the benchmarks on ocaml/ocaml on a weekly basis, but we'd also want to run the benchmarks on PRs to the project. Would it better to take the approach where we add a minimal set of changes to ocaml/ocaml and then use a separate repository like ocaml-benching to store the benchmarks and run them? This is the approach taken by Steve for dune monorepo benchmarks (PR), and this seems like a better approach in the long run. Also, I think we'd not need to do as many tweaks and shenanigans in the UI, etc., as we do in this PR, if we take that approach.

Thoughts @art-w , @ElectreAAS ?

art-w commented 1 year ago

Hm yes I see what you mean with the shenanigans hmhm. I like the idea of configuring on our end that the ocaml/ocaml has a secondary repo ocaml-benching that contains the benchmark code to run, such that we can keep the logic "run a benchmark on every PR / every week" without requiring tricks on the UI side.. It's not entirely obvious but it seems doable?

ElectreAAS commented 1 year ago

I rebased again on main, with the newest API. Hopefully my manual conflict resolutions didn't break anything.

punchagan commented 1 year ago

I like the idea of configuring on our end that the ocaml/ocaml has a secondary repo ocaml-benching that contains the benchmark code to run, such that we can keep the logic "run a benchmark on

I implemented something on these lines in #427

punchagan commented 1 year ago

Closed in favor of #427