Stream: coqbot devs & users

Topic: bench report


view this post on Zulip 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.

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 15:13):

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

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 15:14):

What do you need this for?

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:16):

In order to post a comment

view this post on Zulip 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

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 15:18):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:19):

Who do I call for that?

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 15:19):

Let me check.

view this post on Zulip 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.

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:21):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:21):

Or do you want the refinement now?

view this post on Zulip 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).

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:22):

OK I will use that for now

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:35):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:36):

Is this the right way to go about it?

view this post on Zulip 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.

view this post on Zulip Ali Caglayan (Mar 22 2022 at 15:43):

Alright, I will work on the summary then

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:40):

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

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:40):

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

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:41):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:43):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:44):

Checking just in case

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:47):

Indeed, I don't think so.

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:48):

What is the 7mb file?

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:48):

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

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:49):

it is not essential to include

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:49):

I have a question about fetching artifacts

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:50):

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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:50):

Is that ok or should I be doing it another way

view this post on Zulip 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

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:52):

Yes, it's OK.

view this post on Zulip 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.

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:53):

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

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:57):

Does it have to be html for that?

view this post on Zulip Théo Zimmermann (Mar 22 2022 at 16:57):

I think so.

view this post on Zulip Ali Caglayan (Mar 22 2022 at 16:57):

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

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 18:47):

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

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 22 2022 at 22:14):

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

view this post on Zulip Théo Zimmermann (Mar 23 2022 at 08:20):

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

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Ali Caglayan (Mar 23 2022 at 09:32):

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

view this post on Zulip Ali Caglayan (Mar 23 2022 at 09:32):

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

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip Théo Zimmermann (Mar 23 2022 at 12:21):

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

view this post on Zulip 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).

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Ali Caglayan (Mar 23 2022 at 12:52):

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

view this post on Zulip Théo Zimmermann (Mar 23 2022 at 12:54):

Sure.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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")
+      }
     }
   }
 |}]

view this post on Zulip 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.

view this post on Zulip Théo Zimmermann (Mar 29 2022 at 13:10):

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

view this post on Zulip 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.

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 29 2022 at 13:26):

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

view this post on Zulip Gaëtan Gilbert (Mar 29 2022 at 13:29):

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

view this post on Zulip 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
              ()

view this post on Zulip 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

view this post on Zulip 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?

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 29 2022 at 16:46):

The wrapper I gave is exactly the same behavior as before

view this post on Zulip Ali Caglayan (Mar 29 2022 at 16:46):

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

view this post on Zulip 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

view this post on Zulip Ali Caglayan (Mar 29 2022 at 18:13):

I will need to do some more investigation.

view this post on Zulip Ali Caglayan (Mar 29 2022 at 18:13):

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

view this post on Zulip 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.

view this post on Zulip 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: Jan 31 2023 at 10:01 UTC