-
Notifications
You must be signed in to change notification settings - Fork 527
New Contributor Guide documentation for newcomers to ExecuTorch or open-source #9977
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9977
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e9662f7 with merge base 8d80185 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @jhelsby! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
5faaacb
to
3ef0096
Compare
This is looking really good! Here are some suggestions and potential areas where users might encounter confusion:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can you add a red circle or a cursor on "<> Code" button to show that you clicked that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Larry, many thanks for reviewing. Yes, definitely, thank you!
As I mentioned in the PR description, none of the images have been annotated yet - I wanted to seek opinions on an initial draft first. In the next revision I will annotate all the images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All images have now been annotated where appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Larry, many thanks for reviewing. Yes, definitely, thank you!
As I mentioned in the PR description, none of the images have been annotated yet - I wanted to seek opinions on an initial draft first. In the next revision I will annotate all the images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All images have now been annotated where appropriate.
docs/source/onboarding.md
Outdated
|
||
3. If everything looks good and you are ready for review, click the `Create Pull Request` button. We recommend starting with a draft PR, which will start CI (["Continuous Integration"](https://en.wikipedia.org/wiki/Continuous_integration)) to verify that all tests pass under various configurations before you notify all reviewers and get the first rounds of comments. | ||
|
||
You'll need approval from 3 of our core contributors for your request to be merged. They may have questions or suggestions for you to address or respond to. Be aware that the review process may take a couple of iterations... Nevertheless, this feedback can be helpful for you as you learn more about ExecuTorch or coding best practices from other contributors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byjlw Do we have guidance on number of reviewers? We've largely been operating on 1 review being sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this in the most recent commit to 1 review. Sorry, I copypasted this from another PyTorch contributing guide byjlw@ recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhelsby Thanks for writing this up. It looks great.
@jhelsby can you also add a pointer in CONTRIBUTING.md to this file? You can consolidate this section: https://github.com/pytorch/executorch/blob/main/CONTRIBUTING.md#contributing-workflow to reduce duplicate wording. |
* Added Git info and GitHub account verification * Explained good first issues * Explained origin and upstream differences * Explained syncing forks * Encouraged running tests before and after changes * Gave example branch names and commit messages * Recommended starting with Draft PR, with screenshots * Detailed CI process * Encouraged community engagement * Added TODOs: * Brief build instructions or troubleshooting * Review process timeline and how to proceed if no feedback is received * FAQ
Hi all, I have to stop for the day but have implemented the majority of @byjlw's suggestions in the most recent commit. I also added TODOs for the suggestions I didn't add. I'd be grateful for any clarification on these - I have very limited experience with ExecuTorch and don't think I have the context required to write something meaningful here.
I'll try to implement everyone else's suggestions in the coming days. Thank you so much everyone for the reviews and kind words - I hugely appreciate it. |
@larryliu0820 Many thanks for your comment. I'm sorry, but I'm not totally sure that I follow. Are you suggesting moving the "Contributing workflow" section from CONTRIBUTING.md to this onboarding file? I was under the impression that the documents served very different purposes:
I'm concerned that merging any part of one into the other could make things confusing for both parties - but I may have misunderstood you. Would you possibly be able to expand on your comment? Thank you very much for your time. |
Most open source project uses CONTRIBUTING.md for an entry point to contributors. I'm just suggesting we should add a link to the new doc, in CONTRIBUTING.md. |
Many thanks for clarifying! I just added this in the most recent commit - is this the sort of thing you had in mind? |
Yes! thank you looks good to me. |
Nice work, I looked at your recent change and your note. I think it's good other than the comment i just added which is minor and consider a different name for the file so it's more explict The other stuff can be added over time. Thanks so much! |
2c507bf
to
e9662f7
Compare
Hi all, I've made the final changes. This includes the renaming to Please review and merge if you deem it ready. Thank you all for your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! This will help new folks a lot
docs/source/onboarding.md
Outdated
|
||
Those reviewers/maintainers are here to finetune your contribution and eventually catch some issues before we merge the PR. We aim for this process to be pleasing on both sides: we try to give and get the best. | ||
|
||
Once the reviewers are happy, one of our core contributors will merge your PR. Congratulations — you're now an official ExecuTorch contributor! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once their PR is approved, they should merge it in.
i.e. contributor does the merge once it's been approved. The merge button will be greyed out until it gets approved. Once that happens it's active and they can do the merge themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jhelsby for your contribution to the contributor guide :) This is awesome! |
@pytorchbot cherry-pick --onto release/0.6 -c docs |
Cherry picking #9977The cherry pick PR is at #10110 The following tracker issues are updated: Details for Dev Infra teamRaised by workflow job |
### Summary The New Contributor Guide added in [this PR](#9977) had an inaccuracy, mentioned in [this comment](#9977 (comment)) from @byjlw. The guide mistakenly said that only core contributors can merge PRs. This PR corrects that error. It encourages the reader to merge their PR themselves once approved, and provides illustrative screenshots. Co-authored-by: jhels <11036537+jhels@users.noreply.github.com>
### Summary The New Contributor Guide added in [this PR](#9977) had an inaccuracy, mentioned in [this comment](#9977 (comment)) from @byjlw. The guide mistakenly said that only core contributors can merge PRs. This PR corrects that error. It encourages the reader to merge their PR themselves once approved, and provides illustrative screenshots. Co-authored-by: jhels <11036537+jhels@users.noreply.github.com> (cherry picked from commit eeabc29)
### Summary The New Contributor Guide added in [this PR](#9977) had an inaccuracy, mentioned in [this comment](#9977 (comment)) from @byjlw. The guide mistakenly said that only core contributors can merge PRs. This PR corrects that error. It encourages the reader to merge their PR themselves once approved, and provides illustrative screenshots. Co-authored-by: jhelsby <11036537+jhelsby@users.noreply.github.com>
### Summary The New Contributor Guide added in [this PR](pytorch#9977) had an inaccuracy, mentioned in [this comment](pytorch#9977 (comment)) from @byjlw. The guide mistakenly said that only core contributors can merge PRs. This PR corrects that error. It encourages the reader to merge their PR themselves once approved, and provides illustrative screenshots. Co-authored-by: jhels <11036537+jhels@users.noreply.github.com>
Summary
New Contributor Guide for people new to ExecuTorch, GitHub and/or open-source. Submitting PR as requested in the #executorch-just-works Discord channel.
Note: May need to confirm the
github-action
release notes
label point discussed in the guide - I don't have permission to assign labels, but this may be a permissions issue rather than intentional.Todos For Later
As discussed in the comments, we'd like to make the following additions to this guide over time (not in this PR):
Build Instructions: Since the build process is not detailed here, consider providing a brief overview or common pitfalls users might encounter. This could reduce the number of questions on your Discord server.
Review Process: Clarify the expected timeline for reviews and what contributors should do if they don't receive feedback within a certain period.
Common Issues: Consider adding a FAQ or common troubleshooting section for issues that new contributors frequently encounter.