-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update GitHub Pull Request Template. #13494
base: master
Are you sure you want to change the base?
Update GitHub Pull Request Template. #13494
Conversation
d02b42f
to
beedfaa
Compare
You should be aware of these
All bad memories |
Whoops :D So there was some sort of template before and it was not working as expected? This one is simpler, provides just bullet points for quick update, and clearly states what is needed to process the PR smoothly. We cannot expect people to provide information that we need if we do not communicate the need in the first place :-) What are better alternatives? Sure we can add CI automation to verify if some things are here, then auto-report the request, and repeat until everything is here. But it will take time, CI resources, and frustrate new reporters that will not be provided with clear list of expectations in the first place. How can we improve process on our side? |
Wonder if the checklist should be sent to the PR Submitter via email, upon submitting the PR? So they can edit the PR while it's being compiled (2.25 hours). This probably works better for new Submitters. We can skip the checklist for Experienced Submitters. |
FYI I asked Google Gemini AI to validate a PR based on our proposed PR Requirements: https://gist.github.com/lupyuen/d5f7dd988b610af6144fb00d23c957ad
Google Gemini says:
Wonder if Google Gemini actually understands our requirements? |
@lupyuen Interesting experiment! I suppose one way to find out whether Gemini understands our project requirements could be to give it a PR that does not meet the requirements and see if it gives the correct answer. |
@hartmannathan Remember the PR mentioned by Tomek? https://gist.github.com/lupyuen/99c3602cab74d7380944f0735364c51c#does-this-pr-meet-the-nuttx-requirements
Google Gemini AI thinks that it's OK. Gemini doesn't seem to understand the context (or do we feed to fine-tune our rules?) https://gist.github.com/lupyuen/99c3602cab74d7380944f0735364c51c#response-from-google-gemini-ai
I ran Gemini on the 4 latest PRs... Incomplete PR: Positive ExampleGemini works OK for this Incomplete PR: https://gist.github.com/lupyuen/9b26677cb1d380a8fb66ea7f5c999fab#does-this-pr-meet-the-nuttx-requirements
Incomplete PR: Negative ExampleBut Gemini missed out totally on this Incomplete PR: https://gist.github.com/lupyuen/70d6ff8515005663dd2059de31e23855#does-this-pr-meet-the-nuttx-requirements
Partially-Complete PRsGemini seems helpful for these 2 Partially-Complete PRs:
|
Wow very nice experiment @lupyuen :-) So the AI can provide initial feedback on the PR but we also need to provide requirements list to the AI? Why not give that to the reporter as well? :-) We do not want / need long elaborates just a quick update of points that we need each time PR is reported :-) Is this AI available for free? If so maybe we could attach initial review with feedback that way here with no commit / merge right just as an observer? :-) |
@cederom Yep I think Google Gemini AI is free for low volume? Shall I write a GitHub Bot that will:
Lemme know what you think :-) |
If you only have some spare time @lupyuen that sounds like a great experiment!! =) |
@cederom Yep I'm working on the GitHub Bot now, the Gemini API coding looks easy :-) Update: I'm now testing the NuttX PR Bot: https://github.com/lupyuen/nuttx-pr-bot Here are the PR Reviews posted by our NuttX PR Bot: We are running on the Free Tier of Gemini Pro 1.5, which limits us to 50 PR Reviews Per Day. |
Quick note from me: too much text. Reading the actual change in gitdiff takes much less time than going through the report. Another problem I see is that PRs are not equal. Changes that affect the entire OS and all users are not equal let's say to a cosmetic change in code style. I don't know how reliably the PR impact can be estimated automatically without human interaction. |
We have a comment by @xiaoxiang781216: #13458 (comment)
|
@raiden00pl Thanks for the feedback! I made a tiny tweak to produce more concise output (though we have limited options for tweaking the LLM):
Here's the output of the newer, more concise PR Review Bot: #13524 (comment) (mostly complete PR, less chatty) Why are some PRs ignored by our bot? That's because:
For folks keen to experiment and tweak the LLM:
|
Comment by @acassis: #13518 (comment)
Yep I can do this in my code for the NuttX PR Bot. I'll also check for Multiple Commits and remind folks to Squash Commits. Hopefully this will make things easier for all of us :-) |
Wow, amazing job @lupyuen !!! We can already see improvement in the PR reports :D :D :D I mean trivial changes do not require whole elaborate, I get it. But we have a list of requirements for reporters and now for the AI BOT that will create a nice summary. TO be honest I did not believe it would be possible to achieve in such a short time kudos @lupyuen !!! :-) So we do not want to modify the PR template I understand now, thanks for all of the feedback! Where do we want to put PR example then? CONTRIBUTING.md ? :-) |
One remark @lupyuen - we should run that bot on a separate dedicated github account with no commit / admin rights just in case AI (or someone who controls the AI) wants to modify our codebase :D :D :D |
beedfaa
to
ed1fbe3
Compare
6987593
to
efd70ba
Compare
31118d4
to
b9213b3
Compare
* Update CONTRIBUTING.md guide with hints and examples. * Minor GitHub Pull Request Template update hinting expected information. Signed-off-by: Tomasz 'CeDeROM' CEDRO <tomek@cedro.info>
b9213b3
to
1efeb5d
Compare
There goes the update:
|
too much text for tiny changes actually hurts the review process, especially when it's posted by a bot. |
Thanks @cederom! I made some more fixes to our NuttX PR Bot:
|
I can prepare PR that adds https://github.com/marketplace/actions/pr-size-labeler-action for CI. Each PR will be labeled with the size of the changes. If bot is able to read PR labels, we can ignore small changes. EDIT: as this is external action, it's better to choose a project already used by another Apache project (mynewt): https://github.com/CodelyTV/pr-size-labeler |
@raiden00pl Yep I can make the bot do that. Thanks! |
Hi @raiden00pl thanks for adding the PR Labels! #13542 So our New PRs will have their PR Size labelled as XS, S, M, L or XL. Which PR Size shall we exclude from our PR Review Bot? Thanks! |
Labels are: 'Size: XS', 'Size: S` etc. For now "Size: XS" are changes below 10 lines and these I think should be excluded. I think we can reduce XS size even more, maybe to 5 lines, so simple one-line fixes are excluded (but since we have a character limit per line, "logically" a single-line change can span multiple lines). Default values for labels are:
we'll probably have to adjust them, but they seem good for a start |
Minor: The link for the feedback does not point to this PR directly: [Experimental Bot, please feedback here] (https://github.com/search?q=repo%3Aapache%2Fnuttx+13494&type=pullrequests) |
Summary
Impact
Testing