docs: add contribution guidelines#2530
Conversation
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| Choose the path based on the size and risk of the change: | ||
|
|
||
| - Small, safe changes can go straight to a pull request. | ||
| - Non-trivial changes should start with a GitHub issue. |
There was a problem hiding this comment.
We should add vibecoded implementations without any discussion or explanation will not be reviewed, due to the shear volume of PR requests daily.
The team cannot keep up. Well scoped PRs should be merged.
There was a problem hiding this comment.
We (aiohttp) added some instructions to agents so that they now add to the end of the PR which tool created the PR and which human reviewed it, plus having it open in draft.
End result is that if the PR remains in draft or the comment says "pending human review", we know not to bother looking at the PR and just close it after some inactivity.
|
|
||
| Choose the path based on the size and risk of the change: | ||
|
|
||
| - Small, safe changes can go straight to a pull request. |
There was a problem hiding this comment.
Should we do a number of line changes as a metric to what defines as Small vs Non Trivial
There was a problem hiding this comment.
I think that would be too pedantic? I think the point to do classification here is to encourage communication/transparency. I actually think submitting a plan along with implementation is perfectly fine here as long as the contributor understand that we won't go into the code directly until we agree on the overall plan shape.
| If you use an AI coding agent, ask it to explain: | ||
|
|
||
| - what files it changed and why | ||
| - which issue or discussion scope it followed | ||
| - what checks it ran and the results | ||
| - any risks, assumptions, or follow-up work |
There was a problem hiding this comment.
I feel like if you're asking an AI agent what issue etc. it was working on, it sounds like you're just running AI agent experiments on our repo...
That's just spam. If we want AI agents automatically fixing issues, we'd just set that up ourselves, right?
There was a problem hiding this comment.
this is actually prompt for both agent and human to take responsibility to their PR. similar to what you described in #2530 (comment). Might need better wording
There was a problem hiding this comment.
People are going to use llm to generate text irrespective. I think it is futile to police like that.
Instead providing a standard makes things easier for everyone.
There was a problem hiding this comment.
Although it is true that we have greptile generate PR summaries and description which is well scoped so we shouldn't need another PR description.
Maybe the only thing a user should provide is a Link to the issue they addressed and instructions on how to test their PR in detail.
Rest Greptile should be able to handle.
There was a problem hiding this comment.
People are going to use llm to generate text irrespective. I think it is futile to police like that.
We've banned several users on aio-libs that were clearly just AI bots (with the accounts calling themselves "AI engineers"), and now the level of spam is minimal. So I think it's saved us plenty of time not reviewing stuff that was written and reviewed by nobody. We can generate our own PRs with a bot when we want (which is almost always better quality than these other agents).
| <!-- What you changed and why this approach --> | ||
| <!-- Key design decisions / tradeoffs --> | ||
| <!-- Keep it high-signal; deep planning belongs in the issue. --> | ||
| <!-- What changed and why? Keep this focused; deeper discussion belongs in the linked issue. --> |
There was a problem hiding this comment.
The reason this has been split into Problem/Solution is because a lot of people submit PRs where they set agents to describe in intimate detail every aspect of what they changes (including how many lines they changed in every file) but never bother to say why we'd want to merge the PR to begin with.
Describing what the issue is is the most important part. The summary is not as important because you can deduce that from the the diff
| <!-- Required for non-trivial changes. Small safe fixes may write "N/A". --> | ||
|
|
||
| Closes DIM-XXX | ||
| Closes # |
There was a problem hiding this comment.
We decided a while ago that Linear is the primary source, hence why DIM-XXX is used.
There was a problem hiding this comment.
Does it get auto-closed on merge though?
There was a problem hiding this comment.
But linear is only visible internally right?
There was a problem hiding this comment.
Does it get auto-closed on merge though?
@Dreamsorcerer yes. It auto closes it.
It's pretty sweet.
But linear is only visible internally right?
Yes, that's the idea 😅
| If you use an AI coding agent, ask it to explain: | ||
|
|
||
| - what files it changed and why |
There was a problem hiding this comment.
It's unclear what this refers to. Are you saying people should use LLMs to create PR descriptions? I don't like that. It's annoying when someone submits a PR and explains every single change in every file.
Well written code doesn't need that type of explanation. If something complex is added, it probably should be explained in docs/ or in docstrings, not the PR description.
I'd rather have PR descriptions be human written only. Shorter is better. Agents just throw walls of text.
In other words, when someone wants to make a contribution, something motivated him to make that contribution. That should be what's written in the description, in the contributors own words. There's no need for that to be passed through an LLM.
There was a problem hiding this comment.
| ## Logs / screenshots | ||
|
|
||
| label: What happened? | ||
| description: Describe the bug and its impact. |
There was a problem hiding this comment.
If you look at the history, we've had this long questionnaires before. That change was reverted into something more direct and freeflowing.
You might want to discuss this with @leshy .
There was a problem hiding this comment.
Yeah, these tend to annoy me. There's always cases where the boxes are not relevant. I don't a more structured form as long as nothing is required.
|
|
||
| <!-- oneliner required to run the actual feature --> | ||
| <!-- blueprint for robot changes, benchmarks for transport changes etc --> | ||
| <!-- What checks did you run? Include commands, relevant output, screenshots, or robot/sim notes when useful. --> |
There was a problem hiding this comment.
The key issue here is that we want human instructions to run the actual feature. In this PR you added this:
uv run pre-commit run --files CONTRIBUTING.md .github/pull_request_template.md .github/ISSUE_TEMPLATE/bug_report.yml .github/ISSUE_TEMPLATE/feature_request.yml
This is not useful. All of that runs in CI so there's no reason to mention it here. This PR doensn't change any functionality, so the section should be blank.
This whole thing was started because we wanted a way for reviewers to be able to validate that the feature worked for them. I.e. manual tests, not automated tests.
For example, if the PR implemented spatial memory, the section should look something like this:
Start the blueprint:
uv run --simulation=dimsim dimos run unitree-go2-agenticStart the humancli:
uv run dimos run unitree-go2-agenticTeleop the robot around to the bed room.
Teleop outside the house
Type in humancli: "Go to the bed."
The robot should start moving and go to the bedroom.
The point is to instruct a person how to use the feature, because, for example, I might not know which blueprint I'm supposed to use. It allows a reviewer to easily validate that the feature was implemented correctly. Without this section, a reviewer would have to deeply inspect the code to figure out all of this.
| - Non-trivial changes should start with a GitHub issue. | ||
| - Core architecture changes should start with a GitHub issue or discussion before implementation. | ||
|
|
||
| New contributors can start with issues labeled `good first issue`. |
There was a problem hiding this comment.
On aiohttp, we try to clarify that these are designed for human contributors to learn from, not suitable for AI agents to churn through. Probably makes no difference though..
Linked issue or discussion
Linear: DIM-1021
closes #2529
Summary
Adds a concise contribution SOP for community contributors and refines GitHub templates to improve issue/PR scoping.
Changes include:
CONTRIBUTING.mdValidation
uv run pre-commit run --files CONTRIBUTING.md .github/pull_request_template.md .github/ISSUE_TEMPLATE/bug_report.yml .github/ISSUE_TEMPLATE/feature_request.ymlChecklist
AI assistance used? Yes, drafted and edited with an AI coding agent under human direction.