Fails to comment on huge PRs

Maxim-Mazurok commented 2 years ago

TL;DR: when opening huge PRs, such as https://github.com/DefinitelyTyped/DefinitelyTyped/pull/62106 - mergebot never picks it up.

Someone would need to log into the azure functions to read the logs to know exactly why, but there is a 'too big to review' style check in the bot which this doesn't seem to be getting hit with. Originally posted by @orta in https://github.com/DefinitelyTyped/DefinitelyTyped/issues/62106#issuecomment-1242663828

Other huge PRs that it didn't pick up:

Maxim-Mazurok commented 2 years ago

Hi guys, any idea how to fix this? We've removed // TypeScript Version from types and I'd like to create a PR to update all gapi.client.* DT types, but I'm afraid that mergebot will fail on a huge PR again. I'm happy to still give it a try if that'll help in your investigation. Perhaps @andrewbranch or @elibarzilay could help? Thank you!

elibarzilay commented 2 years ago

sigh ... and just today I deleted my fork of this repo since it was stale :)

Anyway, I tried to run it manually, and the problem is as follows:

The bot grabs the PR information. The result is very big, as you'd expect it to be, but that works as it should.
It also classifies it as too big (2039 files is definitely bigger than the 500 which makes it use that label), so this part is fine too.
But since the above worked as it should, it actually fetched all 2039 paths, and then it hops along its merry way to categorize all of these files...
This chugs along at a pace of around 3 files per second, and I estimate that the total time for this would take just a bit over 10 minutes.
...and as if that's not long enough by itself, it then goes to fetch the old versions of the index files to get the previous owners (not as many as the >2k, but given that it's very few file changes in each package, I'm guessing that it's still "a lot"),
...which really doesn't matter now, since IIRC, the whole thing would abort after 10 minutes.

The GH webhook attention span is very short (maybe ~2m?) which is irrelevant since the function should continue doing its thing which results in the actions visible on the PR, but I think that after 10m the azure function execution gets too bored too, and kills the whole thing, which explains the bot's speechlessness.

@Maxim-Mazurok, the bottom line for you is unfortunately-hopefully unchanged... "Unfortunate" because you'd need to do the splitting anyway to get such changes through in a timely manner, "hopefully" because the delays should come from the person on DT rotation who would need to scan all of the changes to ensure nothing bad in it but given that humans are humans it's likely for that person to look at the first few hundred changes and merge with a shrug. (I know I would do just that :)

In any case, #448 should fix that.

Maxim-Mazurok commented 2 years ago

That all makes sense, I was expecting something along those lines, thank you for looking into that and for the quick hopefully-fix :)

PR I'm going to open will likely be just removing the same line from all packages, so I expect it can be easily mass-reviewed using some combination of git diff and uniq commands, or with an above-mentioned shrug :) I will try to keep non-humanly reviewable PRs to a minimum tho, cheers!

elibarzilay commented 2 years ago

Heh, consider the fact that cloning DT takes a good amount of time and space, and that using plain git to fetch a PR branch from a different repo can be confusing, and it's not surprising that it's not done too often given the volume of PRs.

But yeah, something like git diff ... | grep '^[+-]' | grep -vE '^(\+\+\+|---) [ab]/' | sort -u is probably going to make it easy to scan. Maybe include instructions for how to fetch the branch and run the diff would make it easier to review if you have more of these in the future...

Maxim-Mazurok commented 2 years ago

@elibarzilay the bot didn't comment on this huge PR: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/62446 Even though CI failed, I think it still should've picked it up and commented "The CI build failed", same as in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/62399 Might need to re-open this issue, @andrewbranch Thank you!

elibarzilay commented 2 years ago

I tried to run it locally, and it's still a pretty long run, but it takes 6m, which I thought is under the azure function timeout.

The thing that takes most of the time there, BTW, is getting owners for around 460 packages -- I'm guessing that it's slower because it fetches old files which take more time to recover. It uses the api for that, but ultimately it will end up digging into the git repo to get it all.

Someone (@andrewbranch?) with access to that should look at whether the timeout is shorter than that, and either

Make it longer if it's 5m
Make it grab less than 500 files (TOO_MANY_FILES in pr-query.ts)
Revise the code to make the queries in parallel (but I doubt that it'll help, since it will probably trigger some GH threshold if it does 460 quick api calls)

DefinitelyTyped / dt-mergebot

Fails to comment on huge PRs #447