You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 12 Next »

This page captures the approach we'll follow for streamlining the development process in MXNet project.

Pull Requests

 

As MXNet contributions continue to grow there is a need to scale the capacity for reviewing incoming PRs. While the primary goal is to have a subject matter expert (SME) review the PR and provide timely feedback, a secondary goal is to grow the pool of SMEs and farm out the ownership of reviewing specific PRs to them. As a first step, we will have a list of volunteers who have a demonstrated proficiency and a track record of making quality contributions to various MXNet components as SMEs for those areas.

The following is a proposal for managing PR assignments:

  • Identify a set of MXNet components and for each component a list of reviewers who are willing to review submissions related to that component. For example, the components could be “engine”, “operators”, “tutorials”, “python/scala/R bindings”, etc. Alternatively, since the code base is already organized into logical folders, we can identify a set of reviewers to each source folder..

  • When creating a new PR, the best practice is to label the PR with appropriate components being modified. Optionally, tag the reviewers for those components in the PR.

  • Core MXNet maintainers such as @piiswrong and @mli can always pitch in and provide additional feedback on any PR or tag specific other developers whose feedback might be useful.

Guidelines for Contributors

  • When working on a large project, try to break down contributions into small logical blocks that can go into separate PRs and therefore can be merged incrementally. Avoid creating a single large PR with a huge number of changes.

  • Include an appropriate title and description for your PR. Include links to github issues that are pertinent to this PR. Use the description to inform the reviewer of any details that may be hard to infer just from looking at the code diff.

  • When adding new APIs or modifying existing APIs:

    • Make sure to add/update appropriate API documentation.

    • If adding a new API, include in the API documentation sample code that shows how to use the API.

    • If modifying an existing API, make sure that API documentation and code samples are still accurate after your changes.

  • Changes that add new functionality or extend existing functionality should always be accompanied by unit tests that exercise that functionality.

  • When updating code that is performance sensitive, include in the PR description details on how you tested your changes from a performance standpoint.

  • Please @mention those individuals who you think might be interested in reviewing your PR or otherwise can provide meaningful feedback.

  • If you are actively working on a PR and therefore not immediately looking for feedback, include a [WIP] prefix in the title.

  • If the PR build fails, it is your responsibility to understand why it failed and fix the underlying issue. As a best practice, always ensure you can build your changes locally and that there are no lint errors before opening/updating a PR.

  • When you receive feedback for your PR, try and address the reviewer’s comments/concerns in a timely fashion. If you don’t have time to immediately address them, adding a short note to the PR about when you plan to update the PR will be helpful. Feel free to solicit help if you want others to pitch in.

Guidelines for Reviewers/Committers

  • As a reviewer you have the responsibility to ensure contributions meet the quality bar. This means you should try and ensure:

    • Contributions are inline with the overall code architecture.

    • Contributions are not lowering the overall code maintainability i.e. documentation and unit tests are not missing.

    • If you don’t understand the logic/flow now, chances are it won’t automatically get better in the future. So feel free to push back if code/logic is hard to follow. Ask to include comments clarifying implementation details whenever necessary.

  • When providing feedback or when seeking clarification, be clear. Don’t assume the PR owner knows exactly what is in your mind.

  • If you decide to close a PR without merging, get an ack from the PR owner that that is indeed the right next step.

  • Proposed NOT agreed upon yet: 

    • a) It is NOT recommended for a committer to merge pull requests that the committer authored. Instead the committer MUST get at least one approval from another committer to merge his/her pull request.

    • b) When you update a pull request with upstream, you MUST use rebase to ensure that the pull request is easy to review by the community. See the how-to link here: https://mxnet.incubator.apache.org/community/contribute.html

Deleting Comments on GitHub

GitHub issues and pull requests allow a user with "Write" permissions to delete another's comment. The Apache Way yearns for openness in all situations, deleting a comment is rarely the right thing to do. 

If a comment does need deleting, most likely because it does not adhere to our Code of Conduct, then the PMC as a whole should discuss and vote on the deletion. If a comment needs to be deleted quickly, then either the PMC Chair (or while in the Incubator a Mentor) should make that decision.  

 

  • No labels