Fixed #33862 -- Added workflow to run the ASV benchmarks for labeled PR.

[deepakdinesh1123]

I have added a workflow that runs the benchmarks in ​smithdc1/django-asv against a pull request when it is labeled with the label benchmark. If the performance has not changed significantly, a pull request status message Benchmarking Result - BENCHMARKS NOT CHANGED SIGNIFICANTLY is added, if the performance has decreased a pull request status message Benchmarking Result - SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. PERFORMANCE DECREASED is added.

ticket-33862

[github-actions]

Hello @deepakdinesh1123! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

[carltongibson]

Hey @deepakdinesh1123. Thanks for this.

I need to have a proper look next week, but initial thoughts:

• I had imagined that we'd trigger the workflow in the smithdc1/django-asv repo (wherever that ends up being housed), which would fetch the latest Django commit, and run the benchmarks, committing the results, rather than checking out here. Likely this is just me needing to turn my head the other way around, but 🤔
• At least on the Wiki CI page, if not in the Contributing Guide (somewhere) we should document "Add the benchmark label" to run...", linking to the README there so folks can dig-in.

[deepakdinesh1123]

trigger the workflow in the smithdc1/django-asv repo

Since it required the creation of an ACCESS_TOKEN and the usage of third-party action, after discussions it was decided to add the workflow to django\django itself.

Add the benchmark label" to run..."

Should I edit the Wiki CI page on the website itself and submit the changes?

[carltongibson]

Should I edit the Wiki CI page on the website itself and submit the changes?

Let's wait. The label doesn't exist yet, and it'd be confusing, but you can draft up any notes and post them here, or ..., if you like.

[smithdc1]

... that we'd trigger the workflow in the smithdc1/django-asv repo ...

Yes -- That's how I had it in my head to start with too. However, we ran into access issues and needing to pass tokens about which I'm not sure about. There is this blog post which shows one approach but that seems rather complex.

... committing the results ...

So we can't commit the results with this approach, but I think that's ok. I think the question we're trying to answer with this patch is "in back to back runs, is a PR +ve or -ve". Storing the result of that run is not important, it'll be picked up in the daily run on the django-asv repo.

[carltongibson]

Hey @deepakdinesh1123 — I added the label, and it seems to work so... 👍

I think if you rebase the flake8 errors are already fixed.

[felixxm]

Hey @deepakdinesh1123 — I added the label, and it seems to work so... +1

I'm not sure about that 🤔 in logs, I only see:

· No information stored about machine 'fv-az173-80'. I know about nothing.

[carltongibson]

Ah, yes... actually looking at the run. 😜

(I was looking at That it triggered only)

[deepakdinesh1123]

I tried to set up the machine details in ASV using several different methods but all of them displayed the message
No information stored about machine 'fv-az173-80'. I know about nothing. Not sure how to avoid this though.

[carltongibson]

@deepakdinesh1123 👋

Not sure how to avoid this though.

Given that stdout is going to out.txt, this must be on stderr yes? Can you add a 2> error.txt and check that after the run here:

asv continuous --interleave-processes -a processes=2 --split --show-stderr 'HEAD^' 'HEAD'

… and then we can disgard that if we're happy.

Beyond that, can we echo enough of the output to verify that the benchmarks ran correctly if we look at the logs, maybe with something like the total time as a sanity check? 🤔

[deepakdinesh1123]

Given that stdout is going to out.txt, this must be on stderr yes?

The output of asv continuous --interleave-processes -a processes=2 --split --show-stderr 'HEAD^' 'HEAD' is being piped to sed and is then being sent to out.txt

· No information stored about machine 'fv-az173-80'. I know about nothing. is being displayed due to the command

asv machine --machine ubuntu-latest --yes

It is required before running asv benchmarks on a machine as it asks the user to enter details in interactive mode if not done. Should I redirect the output of this command to some other file so this is not printed on the screen?

the total time as a sanity check

ASV does not have any methods to measure the total time taken to run the benchmarks, Shall I use some other method( time command) to record the time and then add it to the Github step Summary?

[carltongibson]

 asv machine --machine ubuntu-latest --yes

It is required before running asv benchmarks on a machine as it asks the user to enter details in interactive mode if not done. Should I redirect the output of this command to some other file so this is not printed on the screen?

Yes, please. Let's just capture that. 👍

ASV does not have any methods to measure the total time taken to run the benchmarks...

Hmmm. Not sure: need to play but it would be nice to have something like the target time, and the actual time that led to the faster/slower decision (if that makes sense). 🤔 (Anything really: we get the status check, but nice to look to the logs for more detail if possible...) What do you think?

[carltongibson]

@deepakdinesh1123 Also — can you look at why the benchmark workflow isn't triggered for 1f7b44b 🤔

[carltongibson]

OK... If I remove the label and re-add it again, the workflow is triggered.

Ideally it would run for pushes to the PR branch if the label is already applied too. I'm sure that's feasible with the right YAML. 🤔 Can you have a look? (Update: since the benchmarks take a while, we might not want them auto-run on push...)

Thanks

[deepakdinesh1123]

might not want them auto-run on push...

@carltongibson I have added the condition to run the benchmarks when pushes are made to pr with a given label, in order to avoid the auto-run of the benchmarks shall I modify the workflow such that it runs when a comment with the content benchmark is made?

[carltongibson]

By removing the label we're able to skip the benchmarks again. I think that's a nice enough control. Toggle on for any PR where we want to run it, toggle off to disable that for updates, until ready to toggle on again. We can tweak, but I think that's good enough to go.

Thanks for this @deepakdinesh1123 — let's see how it goes over the next period! 👍

[carltongibson]

I added a note to the CI Wiki page: https://code.djangoproject.com/wiki/CI#GitHubactions