From e1d6f7330b5095d9caf48ba1f9de2c267312a28c Mon Sep 17 00:00:00 2001 From: Jakob Bludau Date: Mon, 13 Apr 2026 14:05:32 -0400 Subject: [PATCH 1/9] split developer infor on prs and reviews into separate sections and some (hopefully) useful questions --- docs/source/developer-guides/index.rst | 3 +- .../{prs-and-reviews.rst => prs.rst} | 17 +--- docs/source/developer-guides/reviews.rst | 91 +++++++++++++++++++ 3 files changed, 96 insertions(+), 15 deletions(-) rename docs/source/developer-guides/{prs-and-reviews.rst => prs.rst} (68%) create mode 100644 docs/source/developer-guides/reviews.rst diff --git a/docs/source/developer-guides/index.rst b/docs/source/developer-guides/index.rst index 3857c83e3..e609d9488 100644 --- a/docs/source/developer-guides/index.rst +++ b/docs/source/developer-guides/index.rst @@ -6,5 +6,6 @@ The following pages provide information for existing and prospective developers .. toctree:: :maxdepth: 1 - PRs and Reviews + Creating Pull-Requests Coding Standards + Giving Reviews diff --git a/docs/source/developer-guides/prs-and-reviews.rst b/docs/source/developer-guides/prs.rst similarity index 68% rename from docs/source/developer-guides/prs-and-reviews.rst rename to docs/source/developer-guides/prs.rst index 240213bf4..c40cea867 100644 --- a/docs/source/developer-guides/prs-and-reviews.rst +++ b/docs/source/developer-guides/prs.rst @@ -1,7 +1,7 @@ -PRs and Reviews -=============== +PRs +=== -The goal of a review is to ensure that the proposed change is useful and maintainable over the long run. Submitters should consider and reviewers should evaluate the below criteria. +The goal of the pull-request and review process is to ensure that the proposed change is useful and maintainable over the long run. Submitters should consider and reviewers should evaluate the below criteria. PR Description --------------- @@ -59,14 +59,3 @@ Tests - For bug fix PRs: add test which would catch the issue without the fix - Do newly added tests have the correct granularity? - Do tests have a suitable runtime or are unnecessarily large? - -Reviewer Behavior ------------------ - -- provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. -- only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. -- only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). -- mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. -- contact authors directly if more clarification is needed. -- not be afraid of reviewing pull requests even if they are (slightly) outside their comfort zone. -- work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting. diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst new file mode 100644 index 000000000..8072502b2 --- /dev/null +++ b/docs/source/developer-guides/reviews.rst @@ -0,0 +1,91 @@ +Reviews +======= + +The goal of a review is to help the code contributor to improve the code while also checking if it is the best approach to the described problem. + +Reviewer Behavior +----------------- + +- provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. +- only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. +- only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). +- mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. +- contact authors directly if more clarification is needed. +- don't be afraid of reviewing pull requests even if they are (slightly) outside your comfort zone. +- work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting. + +Checklist +--------- + +The following checklist can serve as guidance for a thorough review. It is extensive and tries to be general, thus it might be overkill for small PRs (e.g. simple bugfixes) + +Fundamental Qestions +-------------------- + +- Is clear what problem the PR is trying to resolve? +- Is the proposed solution appropriate for the the problem? +- Is it working (check the CI)? +- Does the PR introduce/change abstractions? If yes, what behavior has changed? +- Which other abstractions/classes/concepts in Kokkos interact with the change? +- Is there a good reason for including this code? + +Design +------ + +- Does it adhere to design principles like SOLID,DRY? +- Does the design and variable naming fit into the rest of Kokkos? +- Is the current design restricting future design choices? Does/should it allow extension? +- Is any implicit dpenendency introduced? +- Is the design reasonably simple? +- Are the used names descriptive of what something does? + +Complexity +---------- + +- How complex is the change? Should/can it be split into multiple PRs? +- Is there an easier way to do it? +- Should some corner cases be excluded? + +Interface and usecase +--------------------- + +- Is it aligned with other Kokkos interfaces? +- Is the interface of all functions/classes/etc. intuitive? Does it have a descriptive name? +- How is it used in code in the end? + +Documentation +------------- + +- Does it need documentation? If yes, is it already in a PR? +- Does it need comments in the implementation to express intent? + +Interactions +------------ + +- Does an abstraction/function have opaque dependencies on non-local variables? +- Are interfaces or variables hidden or shadowed? +- Are any non-local variables changed? + +Includes +-------- + +- Does my header use minimal includes? +- Are the new headers self-contained? + +Run +--- + +- Does the CI work? +- Can I run it and try to break it? + +Communication +------------- + +- Am I using a helpful, neutral tone? +- Am I giving enough info to the author? + +Final Questions +--------------- + +- Does it definitely improve the current state? +- Am I willing to maintain this code/concept/abstraction/idea in the future? From c02546ce60e0d2c4244b82034119eb9368a56b43 Mon Sep 17 00:00:00 2001 From: Jakob Bludau Date: Mon, 13 Apr 2026 14:12:18 -0400 Subject: [PATCH 2/9] promote checklist to its own header --- docs/source/developer-guides/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index 8072502b2..ec0c387c4 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -15,7 +15,7 @@ Reviewer Behavior - work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting. Checklist ---------- +========= The following checklist can serve as guidance for a thorough review. It is extensive and tries to be general, thus it might be overkill for small PRs (e.g. simple bugfixes) From 496a01d6901409252bb12254263483ec81ac2b64 Mon Sep 17 00:00:00 2001 From: Jakob Bludau <104908666+JBludau@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:30:10 -0400 Subject: [PATCH 3/9] Update docs/source/developer-guides/reviews.rst Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com> --- docs/source/developer-guides/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index ec0c387c4..4261e5e9e 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -23,7 +23,7 @@ Fundamental Qestions -------------------- - Is clear what problem the PR is trying to resolve? -- Is the proposed solution appropriate for the the problem? +- Is the proposed solution appropriate for the problem? - Is it working (check the CI)? - Does the PR introduce/change abstractions? If yes, what behavior has changed? - Which other abstractions/classes/concepts in Kokkos interact with the change? From 1681d2799ce240f4d0de4d498674a35ff7403499 Mon Sep 17 00:00:00 2001 From: Jakob Bludau <104908666+JBludau@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:30:29 -0400 Subject: [PATCH 4/9] Update docs/source/developer-guides/reviews.rst Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com> --- docs/source/developer-guides/reviews.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index 4261e5e9e..7cd37b1fd 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -22,6 +22,7 @@ The following checklist can serve as guidance for a thorough review. It is exten Fundamental Qestions -------------------- +- Is the PR title clear enough about the scope of the changes? - Is clear what problem the PR is trying to resolve? - Is the proposed solution appropriate for the problem? - Is it working (check the CI)? From dd195657a8dc08861cdac932a0e40275a10e7be7 Mon Sep 17 00:00:00 2001 From: Jakob Bludau <104908666+JBludau@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:30:40 -0400 Subject: [PATCH 5/9] Update docs/source/developer-guides/reviews.rst Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com> --- docs/source/developer-guides/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index 7cd37b1fd..18bac97d5 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -70,7 +70,7 @@ Interactions Includes -------- -- Does my header use minimal includes? +- Does the header use minimal includes? - Are the new headers self-contained? Run From ec618c96ed8faa945e3004f96cbeb48307fb458c Mon Sep 17 00:00:00 2001 From: Jakob Bludau <104908666+JBludau@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:31:02 -0400 Subject: [PATCH 6/9] Update docs/source/developer-guides/prs.rst Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com> --- docs/source/developer-guides/prs.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/prs.rst b/docs/source/developer-guides/prs.rst index c40cea867..434bd5d3d 100644 --- a/docs/source/developer-guides/prs.rst +++ b/docs/source/developer-guides/prs.rst @@ -7,7 +7,7 @@ PR Description --------------- - Have a meaningful title: it's easier when creating the changelog or when searching through old PRs -- Motivate why we should merge the PR: adding/changing code risks introducing a new bug. IMO one person asking for a nice to have feature doesn't qualify. +- New features add code that needs to be maintained and tested, and risk introducing bugs. You need to motivate why the PR should be merged - Explain what the PR does in the description: it makes it easier/faster to review - Make the PR as small as possible: it's much easier to review five PRs 200 lines each than one single PR with 1000 lines - If a PR is known to create conflict with other active PRs, try to coordinate, and link to each other From f54d92fc6117dcafda89caeb2f50973535467d3f Mon Sep 17 00:00:00 2001 From: Jakob Bludau Date: Wed, 13 May 2026 16:31:46 -0400 Subject: [PATCH 7/9] capitalize all points --- docs/source/developer-guides/prs.rst | 50 ++++++++++++------------ docs/source/developer-guides/reviews.rst | 14 +++---- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/docs/source/developer-guides/prs.rst b/docs/source/developer-guides/prs.rst index 434bd5d3d..9e53f0f4a 100644 --- a/docs/source/developer-guides/prs.rst +++ b/docs/source/developer-guides/prs.rst @@ -9,28 +9,28 @@ PR Description - Have a meaningful title: it's easier when creating the changelog or when searching through old PRs - New features add code that needs to be maintained and tested, and risk introducing bugs. You need to motivate why the PR should be merged - Explain what the PR does in the description: it makes it easier/faster to review -- Make the PR as small as possible: it's much easier to review five PRs 200 lines each than one single PR with 1000 lines +- Make the PR as small as possible: it's much easier to review five PRs 200 lines each than one single PR with 1000 lines - If a PR is known to create conflict with other active PRs, try to coordinate, and link to each other - - if appropriate, explain the desired review/merge order + - If appropriate, explain the desired review/merge order - For complex changes that will require multiple PRs, create an issue to keep track of what has been done and what's left to do. - Is the PR focused on a single bugfix or feature? - - consider moving self contained changes to separate PRs + - Consider moving self contained changes to separate PRs Public Interfaces ----------------- -- are there comprehensive tests -- does the interface meet the use case - and for that matter is there a use case description - - is the usage intuitive - - do we really want this as a public interface, and maintain it? -- is the interface API consistent with existing ones? - - e.g. if everything else takes execution space instances as the first argument, don't make it the last argument - - does the naming style match other Kokkos APIs -- are the interface semantics consistent with existing ones? - - if everything else (or at least the majority) of interfaces taking an execution space instance argument is async, then new interfaces taking one should be too +- Are there comprehensive tests +- Does the interface meet the use case - and for that matter is there a use case description + - Is the usage intuitive + - Do we really want this as a public interface, and maintain it? +- Is the interface API consistent with existing ones? + - E.g. if everything else takes execution space instances as the first argument, don't make it the last argument + - Does the naming style match other Kokkos APIs +- Are the interface semantics consistent with existing ones? + - If everything else (or at least the majority) of interfaces taking an execution space instance argument is async, then new interfaces taking one should be too - For any routine that's going to launch any parallel work, does it have an overload that takes an execution space instance, and does that overload use the instance for all parallel work? -- if the functionality is similar to an ISO C++ functionality, is the interface and behavior similar, and in places where it's not is it a conscious decision -- is there a corresponding API documentation PR +- If the functionality is similar to an ISO C++ functionality, is the interface and behavior similar, and in places where it's not is it a conscious decision +- Is there a corresponding API documentation PR - Are the corner cases handled (works correctly, won't compile, detected at run time)? - Do the C++ defaulted functions do the right thing (including if needed to be marked with KOKKOS_DEFAULTED_FUNCTION)? @@ -38,20 +38,20 @@ Public Interfaces Internal Implementation ----------------------- -- is the implementation style consistent with the rest of Kokkos -- is there unnecessary code duplication - - in particular: is code that can be shared across backends shared? -- did it go in the right sub-part of Kokkos (core, algorithms, containers etc.) -- are there debug checks we should add? - - like do the arguments make sense together etc. -- are implementation details accidentally exposed? -- is there unnecessary fencing -- are there unnecessary allocations/deallocations +- Is the implementation style consistent with the rest of Kokkos +- Is there unnecessary code duplication + - In particular: is code that can be shared across backends shared? +- Did it go in the right sub-part of Kokkos (core, algorithms, containers etc.) +- Are there debug checks we should add? + - Like do the arguments make sense together etc. +- Are implementation details accidentally exposed? +- Is there unnecessary fencing +- Are there unnecessary allocations/deallocations - Avoiding tangle of inclusions: are headers/files including only what is needed? - Is the code expressing intent clearly (choosing expressive names for variables, functions, etc)? - Are changes in the code and intent properly captured in the description of the PR? -- consider appropriateness of tests for implementation details - - we want to avoid needing to touch many tests for changes in on internal details +- Consider appropriateness of tests for implementation details + - We want to avoid needing to touch many tests for changes in on internal details Tests --------------- diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index 18bac97d5..d90091af1 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -6,13 +6,13 @@ The goal of a review is to help the code contributor to improve the code while a Reviewer Behavior ----------------- -- provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. -- only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. -- only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). -- mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. -- contact authors directly if more clarification is needed. -- don't be afraid of reviewing pull requests even if they are (slightly) outside your comfort zone. -- work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting. +- Provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. +- Only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. +- Only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). +- Mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. +- Contact authors directly if more clarification is needed. +- Don't be afraid of reviewing pull requests even if they are (slightly) outside your comfort zone. +- Work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting. Checklist ========= From 75fe399e36e20da81003dda8f8e144bcffa2872d Mon Sep 17 00:00:00 2001 From: Jakob Bludau Date: Wed, 13 May 2026 16:33:13 -0400 Subject: [PATCH 8/9] fix a typo --- docs/source/developer-guides/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index d90091af1..0e11a99e4 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -19,7 +19,7 @@ Checklist The following checklist can serve as guidance for a thorough review. It is extensive and tries to be general, thus it might be overkill for small PRs (e.g. simple bugfixes) -Fundamental Qestions +Fundamental Questions -------------------- - Is the PR title clear enough about the scope of the changes? From 3177b42e4ae5a07caba23a3f7652fac55c8b2657 Mon Sep 17 00:00:00 2001 From: Jakob Bludau Date: Wed, 13 May 2026 16:36:21 -0400 Subject: [PATCH 9/9] add underline in correct length --- docs/source/developer-guides/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/developer-guides/reviews.rst b/docs/source/developer-guides/reviews.rst index 0e11a99e4..3cfdc1206 100644 --- a/docs/source/developer-guides/reviews.rst +++ b/docs/source/developer-guides/reviews.rst @@ -20,7 +20,7 @@ Checklist The following checklist can serve as guidance for a thorough review. It is extensive and tries to be general, thus it might be overkill for small PRs (e.g. simple bugfixes) Fundamental Questions --------------------- +--------------------- - Is the PR title clear enough about the scope of the changes? - Is clear what problem the PR is trying to resolve?