Update GitHub Pull Request Template.

[cederom]

Summary

• As the number of PRs increases noticeably improvements in review process are necessary.
• Some PRs introduce important and necessary updates but laconic descriptions makes it hard to understand slowing down the review process.
• Some PRs does not fully comply with Contributing guidelines: does not provide sufficient descriptions in commit title / message, does not provide sufficient PR Summary / Impact / Testing information, bundles various functional changes in a single PR, etc. This makes hard to track down changes.
• Some PRs seems to be work-in-progress not yet ready for review.
• I propose minor PR template update with reference to Contributing Guidelines and hints on what information is usually expected in each PR section.
• I propose small updates to Contributing Guidelines and dedicated Example Pull Request Report section as reference input for automated AI PR BOT created by @lupyuen.

Impact

• GitHub Pull Request reporting and review process improvement.
• Brief hints on what information are expected in PR Template.
• Template with standard Pull Request question / description fields and verification checks required by reviewers added to Contributing manual can serve as AI PR BOT input reference.

Testing

• Please verify proposed example template question / description fields and request updates where necessary.
• Thank you @lupyuen for creating the AI PR BOT!! :-)

[patacongo]

You should be aware of these

commit a59ae5536c58510a26e5a64868ea0705d1eb1c2b
Author: David Sidrane <[email protected]>
Date:   Sat Apr 18 05:00:11 2020 -0700

    github: Add PR Template

commit bd7217e21ede8a5f42cd5c63a68f68bffece15df
Author: Gregory Nutt <[email protected]>
Date:   Tue Apr 21 08:18:31 2020 -0600

    Remove boilerplate from the PR template.

    So far, the use of the new pull request template has been disastrous.  People are ignoring the writing instructions the template is polluting the PR history.  This change just removes the boilerplate and writing instructions fromt he template.  We don't need to see this unmodified serveral times per day.

All bad memories

[cederom]

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?

[lupyuen]

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.

[lupyuen]

FYI I asked Google Gemini AI to validate a PR based on our proposed PR Requirements: https://gist.github.com/lupyuen/d5f7dd988b610af6144fb00d23c957ad

# Here are the requirements for a NuttX PR
## Summary ...
## Impact ...
## Testing ...

# Does this PR meet the NuttX Requirements?
## Summary ...
## Impact ...
## Testing ...

Google Gemini says:

Yes, this PR meets the NuttX requirements.
The summary is clear, the impact is well-described and the testing is comprehensive, demonstrating that the changes work as intended and don't introduce regressions.
The addition of the Kconfig option ARCH_MMU_EXT_THEAD allows for flexibility in enabling or disabling the T-Head MMU cache configuration. Overall, the PR is well-prepared and adheres to the NuttX guidelines.

Wonder if Google Gemini actually understands our requirements?

[hartmannathan]

FYI I asked Google Gemini AI to validate a PR based on our proposed PR Requirements: https://gist.github.com/lupyuen/d5f7dd988b610af6144fb00d23c957ad

# Here are the requirements for a NuttX PR

## Summary ...

## Impact ...

## Testing ...



# Does this PR meet the NuttX Requirements?

## Summary ...

## Impact ...

## Testing ...

Google Gemini says:

Yes, this PR meets the NuttX requirements.

The summary is clear, the impact is well-described and the testing is comprehensive, demonstrating that the changes work as intended and don't introduce regressions.

The addition of the Kconfig option ARCH_MMU_EXT_THEAD allows for flexibility in enabling or disabling the T-Head MMU cache configuration. Overall, the PR is well-prepared and adheres to the NuttX guidelines.

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.

[lupyuen]

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

## Summary: Fix warning of unused variable 'devname' ...
## Impact: examples/sotest ...
## Testing: ./tools/configure.sh -l sim:sotest ...

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

Overall Assessment: The PR appears to adhere to the NuttX requirements.
Points to Consider: (1) While the testing demonstrates successful execution, including details about the build host (OS, CPU, compiler) would strengthen the verification process. (2) If the changes touch any configuration options or Kconfig files, mentioning their impact (if any) would be beneficial.
Recommendation: The PR seems suitable for submission, considering the minor nature of the changes and the provided testing information.

I ran Gemini on the 4 latest PRs...

Incomplete PR: Positive Example

Gemini works OK for this Incomplete PR: https://gist.github.com/lupyuen/9b26677cb1d380a8fb66ea7f5c999fab#does-this-pr-meet-the-nuttx-requirements

Overall Assessment:: The PR seems incomplete in its current form. While it introduces a new feature for BCH devices (readonly configuration), several critical details are missing, making it difficult to assess its full impact and adherence to NuttX requirements.
Recommendations: To bring this PR in line with NuttX requirements, consider the following additions:

• Summary: Explain the mechanism of the readonly configuration. If applicable, reference related issues or PRs.
• Impact: Justify the claim of "NONE" impact. If there's truly no impact, explain why.
• Testing:: Provide comprehensive testing details as requested in the template. This includes the build host environment, target(s) used for testing, and testing logs showcasing the feature's behavior.

Incomplete PR: Negative Example

But Gemini missed out totally on this Incomplete PR: https://gist.github.com/lupyuen/70d6ff8515005663dd2059de31e23855#does-this-pr-meet-the-nuttx-requirements

Based on the provided information, this PR appears to be compliant with the NuttX requirements, but it could benefit from slightly more detail.
Points for Improvement

• More detail in the summary: While the summary is concise, it could be enhanced by briefly mentioning the specific macro being removed and why it was deemed useless. This additional context would make the purpose of the PR even clearer.
• Testing logs: Although the "Testing" section confirms that the changes were verified, it lacks specific logs. Even if the change is minor, it's good practice to include at least a brief "before" and "after" snippet to demonstrate that the build and basic functionality are unaffected.

Partially-Complete PRs

Gemini seems helpful for these 2 Partially-Complete PRs:

https://gist.github.com/lupyuen/fee4f889b57dd6a744b8c5de0edbbdf8#does-this-pr-meet-the-nuttx-requirements

The PR seems to meet the NuttX requirements, primarily focusing on configuration simplification. However, the following points need attention:

• Build Host details: Specify the OS and compiler used for testing.
• Documentation: If any user-facing documentation exists around devminor configuration, it might require an update.
• Clarity on the change: While the linked PR might provide details, a concise explanation in this PR's description about the exact change and its benefits would be helpful for reviewers.

https://gist.github.com/lupyuen/77d03e406352424fd7c4235185c5751d#does-this-pr-meet-the-nuttx-requirements

Recommendations:

1. Expand the summary: Provide more details about the specific code changes and how msync is implemented.
2. Review impact: Carefully consider if there are any potential impacts on other areas, even if they seem minor.
3. Provide complete testing information: Include build host details, target information, and actual testing logs showing the behavior before and after the change.

[cederom]

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? :-)

[lupyuen]

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:

1. Watch out for a New PR
2. Validate the PR with Gemini API
3. Post the Gemini Response as a Comment to the New PR

Lemme know what you think :-)

[cederom]

If you only have some spare time @lupyuen that sounds like a great experiment!! =)

[lupyuen]

@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:
#13519 (comment) (partial PR)
#13518 (comment) (partial PR)
#13513 (comment) (incomplete PR)
#13511 (comment) (recently updated)
#13509 (comment) (incomplete PR)
#13499 (comment) (empty PR)
#13458 (comment) (incomplete PR)
#13523 (comment) (incomplete PR)

We are running on the Free Tier of Gemini Pro 1.5, which limits us to 50 PR Reviews Per Day.

[raiden00pl]

#13509 (comment)

Quick note from me: too much text. Reading the actual change in gitdiff takes much less time than going through the report.
One the one hand, the report provides valuable information, but on the other hand, the wall of text is simply discouraging to read.
Maybe a better solution would be to move some of the text to the documentation and attach links in reports ?

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.
When it takes longer to complete the PR content than to make the actual change in code, it can discourage user from contributing.
We have to keep in mind that people are ... lazy, and nothing can be done about it.

I don't know how reliably the PR impact can be estimated automatically without human interaction.
The greater the impact, the greater the requirements for the PR description should be.
Maybe PR labels would be useful for this ? Currently they are practically not used. There are github bots that can help automate this process.

[lupyuen]

We have a comment by @xiaoxiang781216: #13458 (comment)

do we need so heavy process to handle a minor change?

[lupyuen]

Quick note from me: too much text. Reading the actual change in gitdiff takes much less time than going through the report.

@raiden00pl Thanks for the feedback! I made a tiny tweak to produce more concise output (though we have limited options for tweaking the LLM):

# Does this PR meet the NuttX Requirements? Please be concise

Here's the output of the newer, more concise PR Review Bot:

#13524 (comment) (mostly complete PR, less chatty)
#13525 (comment) (incomplete PR, still chatty though)
#13526 (comment) (multiple commits, interesting response)
#13527 (comment) (mostly complete PR, still chatty though)
#13528 (comment) (incomplete PR, kinda chatty)
#13529 (comment) (incomplete PR, chatty)
#13530 (comment) (incomplete PR, less chatty)
#13533 (comment) (incomplete PR, less chatty)

Why are some PRs ignored by our bot? That's because:

1. Our bot only searches for Open PRs (every 10 mins)
2. Our bot skips any PRs with Comments (so it won't loop forever reviewing the same PR)

For folks keen to experiment and tweak the LLM:

1. Browse to Google AI Studio: https://aistudio.google.com/app/u/1/prompts/new_chat
2. At the right, set the Model to "Gemini 1.5 Pro"
3. Copy and paste the LLM Prompt from here (PR Requirements + PR Body): https://gist.github.com/lupyuen/ad3c63c5c8d59c537bb2670c6b2d9416

[lupyuen]

Comment by @acassis: #13518 (comment)

is there a way to highlight the PR is missing commit log message? Maybe we could issue automatically a request change to avoid us to repeating it too

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 :-)

[cederom]

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 ? :-)

[cederom]

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

[lupyuen]

Thanks @cederom! I made some more fixes to our NuttX PR Bot:

1. Check for Empty Commit Messages, as requested by @acassis. Our Bot will say "Please fill in the Commit Message with the PR Summary"

2. Remind folks to Squash Commits: "This PR contains 2 Commits. Please Squash the Multiple Commits into a Single Commit" [Removed this, it's hard for our bot to figure out whether we should squash commits]

3. PR Requirements for NuttX are now easier to update. Just send me the patch here

4. Google Gemini API will sometimes fail with HTTP 500 (not sure why). And the Failed Request will be charged to our Daily Quota! So we retry at most 3 times, in case the PR has some problematic text that the LLM couldn't handle.

5. How does our Bot know when to stop the retries? It uses Emoji Reactions to tag each attempt. So we will see the PR tagged with 🚀 which becomes 👀 which becomes 🚀👀 and then the Bot gives up. (If it's hunky dory, our Bot erases its own Emoji Reactions)

6. Our Bot now runs as GitHub User nuttxpr that has No Write Access to the NuttX Repo. And I won't get spammed by Every NuttX PR in my email :-)

7. Request by @acassis: Our Bot should check if Kconfig is missing the ---help--- text. Right now our Bot doesn't scan any Source Files, because it will get super complicated and might crash. Wonder if we could extend nxstyle to check Kconfig?

[raiden00pl]

too much text for tiny changes actually hurts the review process, especially when it's posted by a 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

[lupyuen]

If bot is able to read PR labels, we can ignore small changes.

@raiden00pl Yep I can make the bot do that. Thanks!

[lupyuen]

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!

[raiden00pl]

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:

xs_label: 'Size: XS', max size = 10
s_label: 'Size: S', max size = 100
m_label: 'Size: M', max size = 500
l_label: 'Size: L', max size = 1000
xl_label: 'Size: XL', > 1000

we'll probably have to adjust them, but they seem good for a start

[tmedicci]

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)

[lupyuen]

The link for the feedback does not point to this PR directly

Thanks @tmedicci! I changed the link so it points to the Search Result for the Issue (that will capture the feedback)

Experimental Bot, please feedback here: https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues

Sorry I can't link directly to the PR or Issue, because GitHub will create a Backlink. And we'll see a very long list of PRs reviewed by the bot. I hope the New Feedback Link is clearer than the Old Feedback Link (which shows a long list of PRs). Thanks :-)

[lupyuen]

Hi All: I have updated our PR Review Bot to Skip PRs of Size XS (<=10 changed lines of code) thanks to @raiden00pl. Please lemme know if we should tune our bot further. Thanks!

[cederom]

@lupyuen would it be possible to also attach bot to nuttx-apps repo? :-)

I can see that quality of PRs here increased noticeably but this also need to happen on nuttx-apps :-)

Thank you!! :-)

[lupyuen]

Hi @raiden00pl could you add the Size Labeler to NuttX Apps? So I can run our PR Bot on NuttX Apps too. Thank you so much :-)

[lupyuen]

@cederom Yep our PR Review Bot now runs on the nuttx-apps repo. Here's an example (that's strangely unchatty): apache/nuttx-apps#2591 (comment)

[cederom]

Are all changes okay and can be merged or I need to update something more? :-)

I would like to provide updated document as reference :-)

[xiaoxiang781216]

Are all changes okay and can be merged or I need to update something more? :-)

I would like to provide updated document as reference :-)

need remove the draft from PR.

[cederom]

need remove the draft from PR.

I think its good to provide short one sentence brief on what we expect in each section. This will be edited out anyway. People still provide empty descriptions, or fill in None in testing, even though there is the AI PR BOT with copy-paste suggestions these fields not get always updated. What others think? :-)

[xiaoxiang781216]

Yes, I am fine with the short description. @lupyuen @raiden00pl @acassis what's your opinion?

[lupyuen]

Yep short description is good. Thanks!

[XuNeo]

Looks like the link to Contributing Guidelines is not valid.
See #14101

[cederom]

Looks like the link to Contributing Guidelines is not valid. See #14101

Thank you @XuNeo !! The fix is here: #14131.