Adding MIOpen Contributing Guide #932
Conversation
aserio
added
help wanted
request_for_comments
|
Open Question: Should we address anything about how the library is licensed in this document? |
CONTRIBUTING.md
Outdated
| . The second reviewer should be a peer reviewer. This reviewer | ||
| can be any other MIOpen developer. | ||
|
|
||
| ## Responsibility of Author |
There was a problem hiding this comment.
Do we have code style guidelines?
There was a problem hiding this comment.
@atamazov, do we have a style available? I did find a page on our wiki related to using Clang Format
There was a problem hiding this comment.
No, we do not have a dedicated coding style doc. Tidy and formatting checks on CI provide the basis for the programming style automatically. I would also recommend reading https://isocpp.org/wiki/faq/coding-standards and https://google.github.io/styleguide/cppguide.html for every MIOpen developer.
atamazov
requested review from
JehandadKhan,
asroy,
ce1adon,
zjing14,
jerryyin,
shurale-nkn and
Slimakanzer
Like this: Any contributed changes must not violate or jeopardize the MIOpen license. For example, it is not allowed to include code snippets from third-party programs that do not allow free use. |
update to resolve comments to revise CONTRIBUTING.md till 05182021 1630 PDT
CONTRIBUTING.md
Outdated
| maintain or improve the overall quality of the codebase. | ||
|
|
||
| Reviewer's task checklist: | ||
| 1. Has the PR passed necessary CI? |
There was a problem hiding this comment.
I want to clarify how we understand "passed necessary CI".
Is it only about tests in current CI which should be passed.
Or also to check that the coverage is sufficient and to ask the author to add tests to the CI, if necessary.
There was a problem hiding this comment.
@shurale-nkn "add tests to the CI" is indicated in Point 5 below, so passing CI here meant tests in current CI which should be passed only.
However, it's a good point about sufficient coverage, and we may have different set of CI for different types of PRs. How would someone new know about which set of CI needs to be passed? :(
There was a problem hiding this comment.
Right now we have a "full set" of tests. If "full set" is passed, then it is Ok.
It is also possible to manually narrow or extend the set or tests. This is also Ok provided that all the reviewers and the author agree on that.
There was a problem hiding this comment.
It may worth explaining this in contributing rules.
There was a problem hiding this comment.
Mostly my question is about unit tests and verification.
And 'full test' can't guarantee any of this.
There was a problem hiding this comment.
Maybe it is the contributor's job to always provide a subsection in the PR description about which subset of test he used for functionality regression. If this is generic changes, then it could be one of the full test stages. If else, a specific test stage need to be specified that can cover the changes.
There was a problem hiding this comment.
Mostly my question is about unit tests and verification. And 'full test' can't guarantee any of this.
If so, then the "full test" has an issue and needs to be updated or fixed.
[Off-topic] BTW this is what you are doing right now with that "reshuffle and reduce test cases", right?
@junliume Yes. If the existing test set does not cover the PR, then the contributor and reviewers should take care of it. Ideally, add necessary tests in the same PR.
There was a problem hiding this comment.
[Off-topic]
BTW this is what you are doing right now with that "reshuffle and reduce test cases", right?
No, it's about our tests do testing only for fastest solution, not for all, so we can't guaranty correctness of all solvers.
I asked to add this feature long time ago. It should be written somewhere in the plans.
Co-authored-by: Artem Tamazov <[email protected]>
Add a contributing guide to the MIOpen repository. Please review the document and provide comments and suggestions below.