Stream: coqbot devs & users

Topic: bench report

Ali Caglayan (Mar 22 2022 at 15:12):

@Théo Zimmermann In job_action how can I get access to the pr_id? I see that I have the pr_num but I can't find how to get the corresponding id.

Théo Zimmermann (Mar 22 2022 at 15:13):

Getting from PR number to PR id unfortunately requires a request to the GitHub API.

Théo Zimmermann (Mar 22 2022 at 15:14):

What do you need this for?

Ali Caglayan (Mar 22 2022 at 15:16):

In order to post a comment

Ali Caglayan (Mar 22 2022 at 15:17):

val post_comment :
     bot_info:Bot_info.t
  -> id:id
  -> message:string
  -> (string, string) result Lwt.t

Théo Zimmermann (Mar 22 2022 at 15:18):

Right. There's no getting around that you need the PR id in this case.

Ali Caglayan (Mar 22 2022 at 15:19):

Who do I call for that?

Théo Zimmermann (Mar 22 2022 at 15:19):

Let me check.

Théo Zimmermann (Mar 22 2022 at 15:20):

If you want to reuse an existing query, you can use get_pull_request_id_and_milestone. Then (as a refinement), we could extract a query that only gets the ID.

Ali Caglayan (Mar 22 2022 at 15:21):

So you mean use that and then change it at later date?

Ali Caglayan (Mar 22 2022 at 15:21):

Or do you want the refinement now?

Théo Zimmermann (Mar 22 2022 at 15:21):

I mean you can use this for your code and I can take care of extracting the right query before merging (unless you do it yourself).

Ali Caglayan (Mar 22 2022 at 15:22):

OK I will use that for now

Ali Caglayan (Mar 22 2022 at 15:35):

@Théo Zimmermann I've drafted a PR: https://github.com/coq/bot/pull/217

Ali Caglayan (Mar 22 2022 at 15:36):

Is this the right way to go about it?

Théo Zimmermann (Mar 22 2022 at 15:40):

Hum, yes I guess. Though the first step (IMHO) would be to just add the reporting as a check run summary. This would already be useful and would not prevent additionally posting a comment next.

Ali Caglayan (Mar 22 2022 at 15:43):

Alright, I will work on the summary then

Ali Caglayan (Mar 22 2022 at 16:40):

@Théo Zimmermann Is there a limit to how much can be put in the summary?

Théo Zimmermann (Mar 22 2022 at 16:40):

There certainly is a limit but I don't know how much.

Théo Zimmermann (Mar 22 2022 at 16:41):

I would expect something similar to the comment limit which is 32kB IIRC.

Ali Caglayan (Mar 22 2022 at 16:43):

Ok so there is no way I can dump a 7mb file in the summary

Ali Caglayan (Mar 22 2022 at 16:44):

Checking just in case

Théo Zimmermann (Mar 22 2022 at 16:47):

Indeed, I don't think so.

Théo Zimmermann (Mar 22 2022 at 16:48):

What is the 7mb file?

Ali Caglayan (Mar 22 2022 at 16:48):

The bench artifacts consist of the speed ups, slow downs, summary and a file listing all line timing changes

Ali Caglayan (Mar 22 2022 at 16:48):

https://gitlab.com/coq/coq/-/jobs/2234949947/artifacts/browse/_bench/timings/

Ali Caglayan (Mar 22 2022 at 16:49):

In this case since I reduced the number of jobs in the bench the file is only 50kb

Ali Caglayan (Mar 22 2022 at 16:49):

it is not essential to include

Ali Caglayan (Mar 22 2022 at 16:49):

I have a question about fetching artifacts

Ali Caglayan (Mar 22 2022 at 16:50):

I am tempted to copy the doc url stuff to do this

Ali Caglayan (Mar 22 2022 at 16:50):

Is that ok or should I be doing it another way

Ali Caglayan (Mar 22 2022 at 16:50):

url |> Uri.of_string |> Client.get
  >>= fun (resp, _) ->
  let status_code = resp |> Response.status |> Code.code_of_status in
  if Int.equal 200 status_code then

Théo Zimmermann (Mar 22 2022 at 16:52):

Yes, it's OK.

Théo Zimmermann (Mar 22 2022 at 16:53):

If some of these more detailed files are available in HTML format, then I would suggest adding links to the HTML artifacts instead of putting them in the summary.

Théo Zimmermann (Mar 22 2022 at 16:53):

(And only including the really essential information in the summary.)

Théo Zimmermann (Mar 22 2022 at 16:54):

Théo Zimmermann said:

Yes, it's OK.

Actually, you may need to store the body of the artifact as well instead of just checking that the HTTP code is 200.

Théo Zimmermann (Mar 22 2022 at 16:55):

And if you can test your artifact fetching code locally (through utop), that would ensure that it doesn't fail unexpectedly once deployed.

Ali Caglayan (Mar 22 2022 at 16:57):

The files are all basically txt files, but I am unsure how to tell gitlab to make them interactively browsable

Ali Caglayan (Mar 22 2022 at 16:57):

Does it have to be html for that?

Théo Zimmermann (Mar 22 2022 at 16:57):

I think so.

Ali Caglayan (Mar 22 2022 at 16:57):

Since 3 of them are small tables, I will post those directly in the summary

Ali Caglayan (Mar 22 2022 at 16:58):

But the fourth would be nice to have in a browsable state, but that will require a change on the coq side

Ali Caglayan (Mar 22 2022 at 18:42):

@Théo Zimmermann I've added a summary page now https://github.com/coq/bot/pull/217

Ali Caglayan (Mar 22 2022 at 18:47):

Deploying for testing: https://github.com/Alizter/bot/runs/5649378771?check_suite_focus=true

Ali Caglayan (Mar 22 2022 at 22:14):

I've managed to do a summary and it looks alright: https://github.com/coq/coq/pull/14748/checks?check_run_id=5651821583

Ali Caglayan (Mar 22 2022 at 22:14):

However I don't know why it is so narrow.

Théo Zimmermann (Mar 23 2022 at 08:20):

That's the way GitHub formats code blocks I think.

Théo Zimmermann (Mar 23 2022 at 08:20):

It would be great if the generated artifacts that are used to display the summary could be in Markdown table format.

Ali Caglayan (Mar 23 2022 at 09:31):

The generated artifacts are straight from the bench scripts: https://github.com/coq/coq/tree/master/dev/bench render_line_results.ml and render_results.ml. They can both easily be modified to also output the data as a GitHub table.

Ali Caglayan (Mar 23 2022 at 09:32):

However I don't think that we would want to do this.

Ali Caglayan (Mar 23 2022 at 09:32):

The tables these scripts create are pretty, compact and easy to read.

Ali Caglayan (Mar 23 2022 at 09:33):

The GitHub markdown tables on the other hand might be a bit of a pain to read.

Théo Zimmermann (Mar 23 2022 at 12:21):

Oh I see that you finally managed to format the code blocks differently. How did you do?

Théo Zimmermann (Mar 23 2022 at 12:21):

Oh I see: you used the details section. Good idea!

Théo Zimmermann (Mar 23 2022 at 12:24):

I agree that the main table is more compact like this that it could ever be in Markdown format. But for the top 25 speedups / slowdowns, there would be a big advantage to use Markdown: you could transform the FILE column into clickable links (such as to: https://coq.gitlab.io/-/coq/-/jobs/2239434488/artifacts/_bench/html/coq-mathcomp-fingroup/mathcomp/fingroup/gproduct.v.html).

Théo Zimmermann (Mar 23 2022 at 12:25):

Since the user time speedup / slowdown is also what most people are interested, extracting just this information as a Markdown table and including it in the summary could also make sense.

Théo Zimmermann (Mar 23 2022 at 12:26):

Anyway, these are only suggestions on making things better. The current state is already really good.

Ali Caglayan (Mar 23 2022 at 12:52):

Those are good ideas, can you create an issue about them?

Théo Zimmermann (Mar 23 2022 at 12:54):

Sure.

Ali Caglayan (Mar 28 2022 at 19:30):

Trying out comments from @coqbot bench https://github.com/coq/coq/pull/14748#issuecomment-1081051970 Let's see if it works.

Ali Caglayan (Mar 29 2022 at 13:02):

@Théo Zimmermann How can I get the url of the check summary? I would like to include it in the comment but I have no idea what it is.

Théo Zimmermann (Mar 29 2022 at 13:04):

The trick is to slightly modify the GraphQL mutation so that it returns something. I'll prepare the beginning of a patch.

Théo Zimmermann (Mar 29 2022 at 13:07):

Here:

diff --git a/bot-components/GitHub_GraphQL.ml b/bot-components/GitHub_GraphQL.ml
index 16f1df6..bba2d9d 100644
--- a/bot-components/GitHub_GraphQL.ml
+++ b/bot-components/GitHub_GraphQL.ml
@@ -328,7 +328,9 @@ module NewCheckRun =
         }
         externalId:$externalId
       }) {
-      clientMutationId
+      checkRun {
+        url @ppxCustom(module: "ParseAsString")
+      }
     }
   }
 |}]

Théo Zimmermann (Mar 29 2022 at 13:09):

Instead of ignoring the result of the mutation (and returning unit), this mutation should return the URL of the resulting check run. You can follow the example of the comment posting mutation that already works like this.

Théo Zimmermann (Mar 29 2022 at 13:10):

(The URL of the comment is currently only used to be displayed in the log.)

Théo Zimmermann (Mar 29 2022 at 13:10):

Eventually, we should probably refactor the whole code so that anytime the bot creates something, it posts its URL to the log.

Ali Caglayan (Mar 29 2022 at 13:25):

Here is the latest version of the comment btw: https://github.com/coq/coq/pull/14748#issuecomment-1081868183

Ali Caglayan (Mar 29 2022 at 13:26):

Seems I have messed up the link, I thought GitHub did [link](url) for links?

Gaëtan Gilbert (Mar 29 2022 at 13:29):

it seems to want a blank line after </details>

Ali Caglayan (Mar 29 2022 at 15:34):

@Théo Zimmermann I had a go at splitting it, though it meant in a lot of places I had to wrap the create_check_run call like this:

              let open Lwt.Syntax in
              let+ _ =
                GitHub_mutations.create_check_run ~bot_info ~name:context
                  ~status ~repo_id ~head_sha:job_info.common_info.head_commit
                  ?conclusion ~title:description ~details_url:job_url
                  ~summary:"" ~external_id ()
              in
              ()

Ali Caglayan (Mar 29 2022 at 15:35):

Eventually we probably want to not do nothing there, or at least print the returned url. let+ is map and let* is bind btw

Théo Zimmermann (Mar 29 2022 at 16:24):

What about just propagating the value until where we actually run the request (no map) and modifying the types instead?

Ali Caglayan (Mar 29 2022 at 16:45):

I'm not sure how to propagate these values. There is more than one, have a look at https://github.com/coq/bot/pull/217/files#diff-c234fddfca49f71b523e6a417a504296214e08c9c42f28575c0ac212f552be3f

Ali Caglayan (Mar 29 2022 at 16:46):

The wrapper I gave is exactly the same behavior as before

Ali Caglayan (Mar 29 2022 at 16:46):

The callers of create_check_run are not really expecting it to return anything

Ali Caglayan (Mar 29 2022 at 18:12):

Hmm it seems the URL for the check summary is not being fetched properly. https://github.com/coq/coq/pull/14748#issuecomment-1082192069

Ali Caglayan (Mar 29 2022 at 18:13):

I will need to do some more investigation.

Ali Caglayan (Mar 29 2022 at 18:13):

The comment conditionally prints the url so that it doesn't block the entire comment.

Ali Caglayan (Mar 29 2022 at 18:25):

29 Mar 2022 18:46:29.869129 <190>1 2022-03-29T17:46:29.485062+00:00 app web.1 - - Pushing status check for bench job.
29 Mar 2022 18:46:31.269297 <158>1 2022-03-29T17:46:30.793718+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=e6418c3c-729f-4f05-96f1-d951b460d49d fwd="140.82.115.251" dyno=web.1 connect=0ms service=1ms status=200 bytes=93 protocol=https
29 Mar 2022 18:46:31.388297 <158>1 2022-03-29T17:46:30.844578+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=48372171-6323-4805-bf56-bab4f65f2712 fwd="140.82.115.102" dyno=web.1 connect=0ms service=2ms status=200 bytes=82 protocol=https
29 Mar 2022 18:46:31.826300 <158>1 2022-03-29T17:46:31.372865+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=dce0b46d-1698-4d96-95eb-f8cc4cf84b47 fwd="140.82.115.110" dyno=web.1 connect=0ms service=2ms status=200 bytes=10618 protocol=https
29 Mar 2022 18:46:32.150297 <158>1 2022-03-29T17:46:31.735316+00:00 heroku router - - at=info method=POST path="/pipeline" host=coqbot.herokuapp.com request_id=f91c067c-1e41-4178-89ec-30a09637fed3 fwd="34.74.226.8" dyno=web.1 connect=0ms service=40ms status=200 bytes=54 protocol=https
29 Mar 2022 18:46:32.622296 <158>1 2022-03-29T17:46:32.250899+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=00ad905f-8a8a-4787-978e-58ea06f90268 fwd="140.82.115.89" dyno=web.1 connect=0ms service=1ms status=200 bytes=82 protocol=https
29 Mar 2022 18:46:32.781297 <158>1 2022-03-29T17:46:32.254793+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=f1741047-60bb-40df-a95a-ef5d2b4d9c74 fwd="140.82.115.116" dyno=web.1 connect=0ms service=2ms status=200 bytes=93 protocol=https
29 Mar 2022 18:46:32.905297 <158>1 2022-03-29T17:46:32.315866+00:00 heroku router - - at=info method=POST path="/github" host=coqbot.herokuapp.com request_id=893b4ee6-bed9-48cd-9474-b84a11dfdc4c fwd="140.82.115.242" dyno=web.1 connect=0ms service=1ms status=200 bytes=82 protocol=https
29 Mar 2022 18:46:37.431218 <190>1 2022-03-29T17:46:37.044836+00:00 app web.1 - - Bench Check Summary URL missing: Error while creating check run.I'm going to look for failed tests to minimize on PR #14748.

Théo Zimmermann (Mar 30 2022 at 12:13):

Ali Caglayan said:

I'm not sure how to propagate these values. There is more than one, have a look at https://github.com/coq/bot/pull/217/files#diff-c234fddfca49f71b523e6a417a504296214e08c9c42f28575c0ac212f552be3f

OK then your changes looks good indeed.

Last updated: May 28 2023 at 18:29 UTC