|
| 1 | +# Contributing to OpenXLA |
| 2 | + |
| 3 | +Everyone can contribute to OpenXLA, and we value everyone’s contributions. There |
| 4 | +are several ways to contribute, including: |
| 5 | + |
| 6 | +* Answering questions on OpenXLA’s discussions forums (openxla-discuss) |
| 7 | + |
| 8 | +* Improving or expanding OpenXLA’s documentation |
| 9 | + |
| 10 | +* Contributing to OpenXLA’s code-base |
| 11 | + |
| 12 | +* Contributing in any of the above ways to the broader ecosystem of libraries |
| 13 | +built on OpenXLA |
| 14 | + |
| 15 | +The OpenXLA project follows |
| 16 | +[Google’s Open Source Community Guidelines](https://opensource.google/conduct/). |
| 17 | + |
| 18 | +## Before you begin |
| 19 | + |
| 20 | +### Sign the Contributor License Agreement |
| 21 | + |
| 22 | +Contributions to this project must be accompanied by a |
| 23 | +[Contributor License Agreement](https://cla.developers.google.com/about) (CLA). |
| 24 | +You (or your employer) retain the copyright to your contribution; this simply |
| 25 | +gives us permission to use and redistribute your contributions as part of the |
| 26 | +project. |
| 27 | + |
| 28 | +If you or your current employer have already signed the Google CLA (even if it |
| 29 | +was for a different project), you probably don't need to do it again. |
| 30 | + |
| 31 | +Visit <https://cla.developers.google.com/> to see your current agreements or to |
| 32 | +sign a new one. |
| 33 | + |
| 34 | +### Review the Code of Conduct |
| 35 | + |
| 36 | +This project follows |
| 37 | +[Tensorflow's Code of Conduct](https://github.com/tensorflow/tensorflow/blob/master/CODE_OF_CONDUCT.md). |
| 38 | + |
| 39 | +## Contribution process |
| 40 | + |
| 41 | +### Developer Guide |
| 42 | + |
| 43 | +For a guide on how to setup a development environment for OpenXLA, including getting |
| 44 | +code, building it, running tests and submitting changes, please refer to the |
| 45 | +[Developer guide](docs/developer_guide.md). |
| 46 | + |
| 47 | +### Code standards |
| 48 | + |
| 49 | +* *Coding style*: We follow [Google's code style guide](https://google.github.io/styleguide/). |
| 50 | + Specifically see the [C/C++](https://google.github.io/styleguide/cppguide.html) and [Python](https://google.github.io/styleguide/pyguide.html) guides. All |
| 51 | + code submitted must strictly conform to these style guides. |
| 52 | + |
| 53 | +* *Compact changes*: We follow |
| 54 | + [Google's engineering practices](https://google.github.io/eng-practices/). |
| 55 | + In particular, please observe the |
| 56 | + [guide on writing compact changes](https://google.github.io/eng-practices/review/developer/small-cls.html). |
| 57 | + Doing so will greatly increase the speed at which you can get your code |
| 58 | + merged due to improve reviewability, and reducing the likelihood of |
| 59 | + unintentional side effects of change. Even if you have a large change, there |
| 60 | + are many strategies for breaking it up into more incremental changes. |
| 61 | + |
| 62 | +* *Test Coverage*: All changes should include appropriate unit tests. Unit |
| 63 | + tests should not be dependent on specific hardware (CPU, GPU, etc.) timings, |
| 64 | + and should make liberal use of mocks and fakes in order to make |
| 65 | + deterministic and focused tests. Changes seeking to extend existing code |
| 66 | + that’s currently hard to test should make appropriate improvements to |
| 67 | + testability. |
| 68 | + |
| 69 | + All changes should include appropriate benchmark results as well in the |
| 70 | + change title to ensure the benefits are clearly understood. |
| 71 | + |
| 72 | +* When in doubt as to conventions within the code, it is always a good idea to |
| 73 | + examine pre-existing code and to try to follow the patterns already in place |
| 74 | + in OpenXLA. |
| 75 | + |
| 76 | + |
| 77 | +### Review Process |
| 78 | + |
| 79 | +All submissions, including submissions by project members, require review. We |
| 80 | +use GitHub pull requests for this purpose. Consult |
| 81 | +[GitHub Help](https://help.github.com/articles/about-pull-requests/) for more |
| 82 | +information on using pull requests. |
| 83 | + |
| 84 | +* Code must follow all standards listed above prior to review. These are not |
| 85 | + optional and it is critical that the submitter ensure their code conforms |
| 86 | + before requesting review in order to assure timely acceptance of changes. |
| 87 | + |
| 88 | +* *All tests must pass*. If you find that a test is broken and the issue is not |
| 89 | + related to your build environment or otherwise your changes, please contact |
| 90 | + the maintainers. |
| 91 | + |
| 92 | +* Try to avoid scope creep during the review process. This is the |
| 93 | + responsibility of both the submitter and the reviewer. If a change starts to |
| 94 | + get too large, consider breaking it up into multiple changes. |
| 95 | + |
| 96 | +* Before a change is merged, it will undergo internal testing that uses code |
| 97 | + internal to Google and other hardware vendors. This can potentially add extra |
| 98 | + steps to the review process if there are failures on internal tests that our |
| 99 | + public CI doesn't catch. The Googler reviewing your change will communicate |
| 100 | + any internal test failures and describe what needs to be fixed. |
| 101 | + |
| 102 | + |
| 103 | +## Frequently asked questions (FAQ) |
| 104 | + |
| 105 | +### "This infrastructure change is not related to my PR. Why should I do it?" |
| 106 | + |
| 107 | +The XLA team doesn't have a dedicated infrastructure team, so it's up to us all |
| 108 | +to build helper libraries and avoid technical debt. We consider it to be a |
| 109 | +regular part of making changes to XLA, and everyone is expected to participate. |
| 110 | +We generally build infrastructure as needed when writing code. |
| 111 | + |
| 112 | +XLA reviewers may ask you to build some infrastructure (or otherwise make a |
| 113 | +large change to a PR) along with a PR that you've written. This request may seem |
| 114 | +unnecessary or orthogonal to the change you're trying to make. This is likely |
| 115 | +because of a mismatch between your expectations about how much infra you need to |
| 116 | +build and your reviewer's expectations for the same. |
| 117 | + |
| 118 | +A mismatch in expectations is okay! That's expected when you're new to a project |
| 119 | +(and it sometimes even happens to us old hats). It's likely that projects you've |
| 120 | +worked on in the past have different expectations. That's also okay and |
| 121 | +expected! It doesn't mean either one of these projects has the wrong approach; |
| 122 | +they're just different. We invite you to take infra requests alongside all other |
| 123 | +review comments as an opportunity to learn what we expect on this project. |
| 124 | + |
| 125 | +### "Can I address your comment in a future PR?" |
| 126 | + |
| 127 | +A frequent question with respect to infrastructure requests (or other large |
| 128 | +requests) in PRs is whether or not the change must be made in the original PR, |
| 129 | +or whether it can be done as a follow-up in a future PR. |
| 130 | + |
| 131 | +In general, XLA does not allow PR authors to address review comments with a |
| 132 | +follow-up PR. When a reviewer decides that something needs to be addressed in a |
| 133 | +given PR, we generally expect authors to address it in that PR, even if what's |
| 134 | +requested is a large change. This standard applies externally and also |
| 135 | +internally within Google. |
| 136 | + |
| 137 | +There are a few reasons that XLA takes this approach. |
| 138 | + |
| 139 | +* *Trust:* Having earned the reviewer's trust is a key component. In an |
| 140 | + open-source project, contributors can appear or disappear at will. After we |
| 141 | + approve a PR, reviewers have no way to ensure that any promised follow-ups |
| 142 | + actually get done. |
| 143 | + |
| 144 | +* *Impact on other developers:* If you have sent a PR touching a particular |
| 145 | + part of XLA, there's a good chance other people are looking at the same |
| 146 | + part. If we accept technical debt in your PR, then everyone who's looking at |
| 147 | + this file will be impacted by this debt until the follow-up is submitted. |
| 148 | + |
| 149 | +* *Reviewer bandwidth:* Deferring a change to a follow-up imposes multiple |
| 150 | + costs on our already overloaded reviewers. Reviewers will probably forget |
| 151 | + what the first PR was about while waiting for the follow-up, making the next |
| 152 | + review more difficult. Also, reviewers will have to keep track of expected |
| 153 | + follow-ups, making sure that they actually happen. If the change can be made |
| 154 | + such that it is truly orthogonal to the original PR so that some other |
| 155 | + reviewer could review it, bandwidth would be less of a problem. In our |
| 156 | + experience, this is rarely the case. |
0 commit comments