Skip to content
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

docs: Added initial PR template with directions for doc only changes and squash merges [no ci] #7700

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

nicolasperez19
Copy link
Contributor

Summary

Changes Made

  • Added directions to the README so that collaborators follow the PR template when sending a PR
  • Added a general pull request template
    • Suggests to collaborators to add [no ci] to the commit title
    • Suggests to collaborators to format commit titles <module>:<commit title> (#<issue_number>) when squashing multiple commits on merge

Additional Information

  • The general PR template is very rough, so adding changes is welcome!

@github-actions github-actions bot added the devops improvements to build systems and github actions label Jun 2, 2024
@mofosyne mofosyne self-requested a review June 2, 2024 23:29
@mofosyne mofosyne added documentation Improvements or additions to documentation need feedback Testing and feedback with results are needed Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels Jun 2, 2024
@mofosyne
Copy link
Collaborator

mofosyne commented Jun 3, 2024

Nice, I be interested to see if everyone else think it's good.

I would like to also add that I've been adding 'review complexity' manually, but it be good to get the submitter to indicate their subjective take on how hard it would be to review their PR. The idea is that at the very least it would speed up my manual assessment and labeling of PR... and also that later we can add some automation to do the PR complexity label automagically.

Did a bit of research and found this option out:

We are currently using actions/labeler for labeling PRs but it can only do it based on content and not on PR title and description.

In this case however to keep this PR complexity down, don't try to add this automation in. We just need to make sure we can add a regex matching process for it in the future somehow.

Approving, but waiting for others to see if we need to flesh it out. I think it's good to keep this simple however.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add a CONTRIBUTING file in the repo and the PR template to just reference the file:

Before opening the PR make sure to read CONTRIBUTING

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/pull_request_template.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE/pull_request_template.md Outdated Show resolved Hide resolved
@nicolasperez19
Copy link
Contributor Author

I made some modifications following everyone's advice.

  • I added a field to the PR template for the review complexity level so that it's easier for @mofosyne to triage PRs.
  • I moved most of the contribution guidelines to its own markdown file
  • In the PR template I referenced ggml-ci and the test commands in the tests folder, and gave an example command following @ggerganov's adivce

@nicolasperez19
Copy link
Contributor Author

I took out most of the fluff from the PR template and moved it to CONTRIBUTING

@mofosyne mofosyne added the merge ready indicates that this may be ready to merge soon and is just holding out in case of objections label Jun 5, 2024
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@slaren
Copy link
Collaborator

slaren commented Jun 9, 2024

It would be nice if there was a brief explanation of llama-bench, compare-llama-bench.py and compare-commits.sh for PRs that affect performance. It's not very easy to find information about these tools without looking at other PRs, and many people are still using main to test performance. Basically, if the PR is meant to improve performance, it is recommended to include the results performance tests using llama-bench or its related tools.

@mofosyne
Copy link
Collaborator

mofosyne commented Jun 9, 2024

@slaren I'm not sure @nicolasperez19 would be familiar enough about these, so you may want to add some pointers where he should read up to write about it.

I've adjusted the PR a bit to include GG's and my preferred changes. As you got some extra feedback, I'll hold back again from merging to give @nicolasperez19 a chance to respond.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good first step - we'll expand after we merge it. Thanks

@mofosyne mofosyne merged commit 57bf62c into ggerganov:master Jun 9, 2024
1 check passed
@mofosyne
Copy link
Collaborator

mofosyne commented Jun 9, 2024

@nicolasperez19 now merged in. Feel free to explore slaren suggestion in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops improvements to build systems and github actions documentation Improvements or additions to documentation merge ready indicates that this may be ready to merge soon and is just holding out in case of objections need feedback Testing and feedback with results are needed Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants