Skip to content
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

Using coqbot to run OCaml benchmarks #288

Open
5 tasks
Zimmi48 opened this issue Jul 6, 2023 · 3 comments
Open
5 tasks

Using coqbot to run OCaml benchmarks #288

Zimmi48 opened this issue Jul 6, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@Zimmi48
Copy link
Member

Zimmi48 commented Jul 6, 2023

The OCaml project has two benchmark suites. One is Sandmark and the other is the ocurrent bench. The OCaml developers would like to have better support for triggering and reporting these benchmarks, and we have discussed some plans with Tarides engineers Shakthi Kanan, Puneeth Chaganti, and Riku Silvola.

Currently, the way of running Sandmark is to edit a configuration file in https://github.com/ocaml-bench/sandmark-nightly-config, to point to a branch that will then be tested every night. There is no reporting mechanism beyond the Sandmark dashboard itself, and developers use screenshots to report results to PR discussions. The ocurrent bench already has support for triggering a bench on a PR with a label and reporting the results as a status check to GitHub, using the ocaml-benchmarks GitHub app. While this could be improved as well, the priority is first to get something for Sandmark.

Ideally, the OCaml developers (and other interested OCaml projects) would use an OCaml-specific bot / GitHub app for this usage, but we decided that using the coqbot-app / @coqbot commands would be fine for a prototype.

We also decided that it would be better to maintain and deploy the code for these features directly from the upstream coqbot master branch instead of creating a fork or a new bot that would reuse part of the codebase only, but would need to be maintained separately.

Finally, there are the options of deploying two separate bots running on different servers from the same code but using different configuration files (currently https://github.com/coq/bot/blob/master/coqbot-config.toml), or of deploying a single instance that would be able to manage several GitHub apps. In any case, we will start from the single GitHub app (coqbot-app) and the single deployed instance as things are set up today.

  • Disable GitLab synchronization by default. #160
  • Add support for @coqbot bench command or similar for the OCaml repository to trigger Sandmark bench.
    The idea is that running this command will make coqbot push a commit to https://github.com/ocaml-bench/sandmark-nightly-config to edit the configuration files to include an entry pointing to the archive for the specific commit on which the bench was requested. The bot could take care of removing the entry when the bench results are posted, which could be done as a later step. BTW, how hard would it be to modify the configuration setup of Sandmark so that entries can be added to separate files? (This would make adding and removing configuration entries much easier, with less risk of conflicts.)
  • Add support for reporting results back from Sandmark to GitHub, as a comment and/or a GitHub Check.
    This requires adding a call to curl in the Sandmark infrastructure so that the bot receives the information that a benchmark has finished and what it needs to print. We would add a dedicated endpoint to the bot, similar to what was done for the auto-minimization feature. The http request would either contain the full information that needs to be reported on GitHub, or the full information could be retrieved by the bot by fetching additional resources.
  • Add support for multi-app support from a single bot deployed instance or set up a separate deployment for the ocaml-bot version.
  • Extend support to the ocurrent bench.
@ticket-tagger ticket-tagger bot added the enhancement New feature or request label Jul 6, 2023
@Zimmi48
Copy link
Member Author

Zimmi48 commented Jul 6, 2023

To provide some pointers. Here is the place where the endpoint for GitHub webhooks is defined:

bot/src/bot.ml

Lines 204 to 210 in 77b3898

| "/push" | "/pull_request" (* legacy endpoints *) | "/github" -> (
body
>>= fun body ->
match
GitHub_subscriptions.receive_github ~secret:github_webhook_secret
(Request.headers req) body
with

Here is the branch of the match used for new comments:

bot/src/bot.ml

Lines 330 to 333 in 77b3898

| Ok (signed, CommentCreated comment_info) -> (
let body =
comment_info.body |> Helpers.trim_comments |> strip_quoted_bot_name
in

And here is the place where the bench comments for the Coq repository are currently handled:

bot/src/bot.ml

Lines 432 to 470 in 77b3898

else if
string_match
~regexp:
(f "@%s:? [Bb]ench native" @@ Str.quote github_bot_name)
body
&& comment_info.issue.pull_request
&& String.equal comment_info.issue.issue.owner "coq"
&& String.equal comment_info.issue.issue.repo "coq"
&& signed
then (
(fun () ->
action_as_github_app ~bot_info ~key ~app_id
~owner:comment_info.issue.issue.owner
~repo:comment_info.issue.issue.repo
(run_bench
~key_value_pairs:[("coq_native", "yes")]
comment_info ) )
|> Lwt.async ;
Server.respond_string ~status:`OK
~body:(f "Received a request to start the bench.")
() )
else if
string_match
~regexp:(f "@%s:? [Bb]ench" @@ Str.quote github_bot_name)
body
&& comment_info.issue.pull_request
&& String.equal comment_info.issue.issue.owner "coq"
&& String.equal comment_info.issue.issue.repo "coq"
&& signed
then (
(fun () ->
action_as_github_app ~bot_info ~key ~app_id
~owner:comment_info.issue.issue.owner
~repo:comment_info.issue.issue.repo
(run_bench comment_info) )
|> Lwt.async ;
Server.respond_string ~status:`OK
~body:(f "Received a request to start the bench.")
() )

Then, the run_bench function is called. A lot of its implementation is specific to the way we do benchmarks for the Coq repository (on GitLab), but the beginning that retrieves the commits for the pull request would also be useful here:

bot/src/actions.ml

Lines 2810 to 2830 in 77b3898

let run_bench ~bot_info ?key_value_pairs comment_info =
(* Do we want to use this more often? *)
let open Lwt.Syntax in
let pr = comment_info.issue in
let owner = pr.issue.owner in
let repo = pr.issue.repo in
let pr_number = pr.number in
(* We need the GitLab build_id and project_id. Currently there is no good way
to query this data so we have to jump through some somewhat useless hoops in
order to get our hands on this information. TODO: do this more directly.*)
let* gitlab_check_summary =
GitHub_queries.get_pull_request_refs ~bot_info ~owner ~repo
~number:pr_number
>>= function
| Error err ->
Lwt.return_error
(f
"Error while fetching PR refs for %s/%s#%d for running bench job: \
%s"
owner repo pr_number err )
| Ok {base= _; head= {sha= head}} ->

@avsm
Copy link

avsm commented Jul 7, 2023

Great to see this progressing! I've created an issue on the OCaml infrastructure tracker for the eventual deployment of such a bot within OCaml for other uses as well:
ocaml/infrastructure#57

Regarding the plan above specifically for benchmarking, is Coqbot definitely needed for the configuration triggering via a push? Another approach is to just have a benchmark-requested label in the target repository, which is added by a repository maintainer. This has the advantage of not creating more email traffic (which a comment does), and easily tracked via a GraphQL query.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Jul 10, 2023

Regarding the plan above specifically for benchmarking, is Coqbot definitely needed for the configuration triggering via a push? Another approach is to just have a benchmark-requested label in the target repository, which is added by a repository maintainer. This has the advantage of not creating more email traffic (which a comment does), and easily tracked via a GraphQL query.

We discussed the two options in the call. The advantage of a comment would be that it would trigger the bench for one specific version of a PR whereas the label would signal intent to bench continuously each new version (unless we auto-remove the label, which we could also do, because this is something we support for another use case in Coq).

The choice to use a label doesn't mean that we cannot use coqbot to implement the triggering, as coqbot can detect label changes thanks to webhooks. On the other hand, if the goal is just to poll which PRs have this label once per day (without really caring of what was the exact state of the PR when it was added), then the method you propose based on a query would work just fine as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants