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

contrib : refresh #10593

Merged
merged 5 commits into from
Dec 2, 2024
Merged

contrib : refresh #10593

merged 5 commits into from
Dec 2, 2024

Conversation

ggerganov
Copy link
Owner

@ggerganov ggerganov commented Nov 30, 2024

  • Clarify test-backend-ops
  • Simplify PR template
  • Add CODEOWNERS

@github-actions github-actions bot added the devops improvements to build systems and github actions label Nov 30, 2024
Copy link
Collaborator

@JohannesGaessler JohannesGaessler left a comment

Choose a reason for hiding this comment

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

Should we add a checkbox about whether or not a contributor will be available long-term for maintaining their code?

CONTRIBUTING.md Outdated
@@ -1,9 +1,9 @@
# Pull requests (for contributors)

- Test your changes:
- Using the commands in the [`tests`](tests) folder. For instance, running the `./tests/test-backend-ops` command tests different backend implementations of the `ggml` library
- Run the `test-backend-ops` tool to validate different backend implementations of the `ggml` operators
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Run the `test-backend-ops` tool to validate different backend implementations of the `ggml` operators
- If you modified GGML, run the `test-backend-ops` tool to check whether different backend implementations of the GGML operators produce consistent results (this requires access to at least two different GGML backends). If you modify an operator or add a new one, add the corresponding test cases.

@ggerganov
Copy link
Owner Author

Should we add a checkbox about whether or not a contributor will be available long-term for maintaining their code?

I think we can add a bullet to the CONTRIBUTING.md that we hope and appreciate if contributors will be available for long-term support of their changes. A checkbox might make it a bit too formal. If other collaborators agree that the checkbox would be a good idea, we will add it.

@JohannesGaessler
Copy link
Collaborator

A bit off-topic, but what is our preferred way to write ggml/GGML/ggml in documentation? When I updated the MNIST example I went with the way it's written in the README, which at the time was "GGML". Currently the linked "Introduction to ggml" consistently uses "ggml". (I don't really have a preference.)

@ggerganov
Copy link
Owner Author

I prefer to use ggml and llama.cpp - all-lowercase. I find the accent is helpful and add it to the main subjects of a comment, so either ggml or ggml is fine.

I use GGUF all-capital because this is the way it was introduced and proposed by the community.

@ngxson
Copy link
Collaborator

ngxson commented Nov 30, 2024

Should we add a checkbox about whether or not a contributor will be available long-term for maintaining their code?

I think we can also take advantage of codeowners file, which automatically ping someone when new PR is added in certain parts of the project

@JohannesGaessler
Copy link
Collaborator

I was thinking about it more in terms of negotiation. My perspective is that a large part of the work in software development lies in maintaining code after the initial implementation. I am actively looking out for issues caused by code that I myself wrote and try to provide fixes in a timely fashion (since it's difficult for me to find the time and motivation to help with other areas of maintenance such as reviewing PRs that are not related to anything I'm already doing). As such I am extremely wary when it comes to merging PRs that would increase maintenance disproportionally to the benefit they provide, in such cases I am generally only willing to merge them if they come with a promise to help with maintenance. It is much easier to secure a commitment if you make it a precondition for the thing that the other party wants (merging the PR) instead of asking for it after the fact. Though these cases are I think rare enough that I think it's also fine to just ask informally if appropriate.

@slaren
Copy link
Collaborator

slaren commented Nov 30, 2024

IMO even the "I have read the contributing guidelines" checkbox is useless. You can very easily tell that many contributors never read them because they don't format the PR title in the way the guidelines ask.

Checkboxes don't mean anything. The only way you can trust that a contributor will be available to maintain their code is if they have an history of doing so.

@ggerganov ggerganov merged commit 4cb003d into master Dec 2, 2024
1 check passed
@ggerganov ggerganov deleted the gg/contrib-refresh branch December 2, 2024 06:53
tinglou pushed a commit to tinglou/llama.cpp that referenced this pull request Dec 7, 2024
* contrib : refresh

* contrib : expand [no ci]

* contrib : expand test-backend-ops instructions

* contrib : add CODEOWNERS

* prs : update template to not have checkbox [no ci]
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants