From 977372b770ad2ef5cc699059bffbabf2d91da0e8 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Sun, 27 May 2018 07:55:57 +0200 Subject: [PATCH 1/7] Update CONTRIBUTING.md --- CONTRIBUTING.md | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 71a3874514..ba4ff5e120 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,7 +50,7 @@ Once your changes and tests are ready to submit for review: 1. Submit a pull request - Push your local changes to your forked copy of the repository and [submit a pull request](https://help.github.com/articles/about-pull-requests/). In the pull request, choose a title which sums up the changes that you have made, and in the body provide more details about what your changes do. Also mention the number of the issue where discussion has taken place, eg `Closes #123`. + Push your local changes to your forked copy of the repository and [submit a pull request](https://help.github.com/articles/about-pull-requests/). In the pull request, choose a title which sums up the changes that you have made, and in the body provide more details about what your changes do. Also mention the number of the issue where discussion has taken place, eg `Closes #123`. See more information about pull requests [below](#pull-requests) Then sit back and wait. There will probably be discussion about the pull request and, if any changes are needed, we would love to work with you to get your pull request merged into ml-cpp. @@ -64,3 +64,45 @@ Please adhere to the general guideline that you should never force push to a pub 1. If you change code, follow the existing [coding style](STYLEGUIDE.md). 1. Write a test, unit tests are located under `lib/{module}/unittest` 1. Test your changes (`make test`) + +## Pull Requests + +Every change made to ml-cpp must be held to a high standard, Pull Requests are equally important as they document changes and decissions that have been made. `You Know, for Search` - a descriptive and relevant summary of the change, helps people to find your PR later on. + +The followings lists some guide lines when authoring pull requests. Note: Try to follow this guideline as close as possible but do not feel bad if you are unsure, team members happily help you. + +1. PR title summarizes the change, short and descriptive. + 1. Use prefixes for quick categorization: + 1. [X.Y] Branch label if backport PR, if ommited the PR only applies to master. Note: Backport PR's are done after master PR, further explaination can be found below [#backport] + 1. [ML] mandatory prefix, to be consistent with other repositories + 1. [FEATURE] If your pull requests targets a feature branch, this prefix helps as a filter +1. A detailed summary of what changed. Some hints: + 1. Keep it short but do not ommit important details. Usually we squash and merge, write what you would write as commit message of the squashed commit + 1. A link to each issue that is closed by the PR (e.g. Closes #123) or related (Relates #456) + 1. Further information if necessary, maybe you want to share a screenshot, list open Todo's, describe your thought process, etc. + 1. Optional: List of backport PR's, other related PR's +1. Label the PR, not all might apply: + 1. `:ml` mandatory label, to be consistent with other repositories + 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement` + 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version + 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature + 1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption + +### Backport + +There needs to be a link in the comments to the originating PR - for easy click through. + +We must create the backport PR at the same time as merging the original to master, although the backport may be merged later. + +Backport PRs do not need to be reviewed, although you should if the backport becomes very different. + +Once we have it in place (soon hopefully), we will wait for the CI to succeed before merging the backport. +In the meantime, BuildMyBranch should still be used even for backports. +(The downstream impact of build failures cannot be underestimated. There has been recent pressure on the es team to do everything possible to make CI green again, so it's a highlighted topic in other teams right now.) + +Backport PRs will be labelled with a new label - `backport`. +(Note - if we decide to create a backport tool, then would love to move across labels e.g. bug, ehnancement) + +Originating PR will be labelled with the full set, as current - `v6.x` `x7.0.0` `bug` `:ml` `analytics` + +Backport check boxes in the originating PR or issue are a great idea, but can be left to personal choice. Because the originating PR/issue may get closed before the backports, I worry they will not get reviewed. From 32e77d081eae3ad52696763571fab593ea287298 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Sun, 27 May 2018 09:14:52 +0200 Subject: [PATCH 2/7] Update CONTRIBUTING.md --- CONTRIBUTING.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba4ff5e120..47d396210e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,11 +82,16 @@ The followings lists some guide lines when authoring pull requests. Note: Try to 1. Further information if necessary, maybe you want to share a screenshot, list open Todo's, describe your thought process, etc. 1. Optional: List of backport PR's, other related PR's 1. Label the PR, not all might apply: - 1. `:ml` mandatory label, to be consistent with other repositories - 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement` - 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version + 1. `:ml` mandatory label, to be consistent with other repositories. + 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`. + 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version. 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature - 1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption + 1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption. + 1. `discuss` If your PR suggests a change which you first like to discuss regarding it's functional changes before going deep into actual implementation details (e.g. you change a default) + 1. `WIP` let's potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. + 1. `review` Try to find reviewers for your change, github has some suggestions, however if unsure or you know the reviewer is very busy, `review` marks the PR as free to grab for review. Still, PR's are open to anyone to comment. + + ### Backport From 5085698449d45e450a0b1bb3e94b030219da45dd Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Mon, 28 May 2018 09:45:40 +0200 Subject: [PATCH 3/7] Update CONTRIBUTING.md --- CONTRIBUTING.md | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 47d396210e..fcd4ae3b6b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -67,9 +67,9 @@ Please adhere to the general guideline that you should never force push to a pub ## Pull Requests -Every change made to ml-cpp must be held to a high standard, Pull Requests are equally important as they document changes and decissions that have been made. `You Know, for Search` - a descriptive and relevant summary of the change, helps people to find your PR later on. +Every change made to ml-cpp must be held to a high standard, Pull Requests are equally important as they document changes and decissions that have been made. `You Know, for Search` - a descriptive and relevant summary of the change helps people to find your PR later on. -The followings lists some guide lines when authoring pull requests. Note: Try to follow this guideline as close as possible but do not feel bad if you are unsure, team members happily help you. +The followings lists some guide lines when authoring pull requests. Note: Try to follow this guideline as close as possible but do not feel bad if you are unsure, team members can help you. 1. PR title summarizes the change, short and descriptive. 1. Use prefixes for quick categorization: @@ -77,37 +77,29 @@ The followings lists some guide lines when authoring pull requests. Note: Try to 1. [ML] mandatory prefix, to be consistent with other repositories 1. [FEATURE] If your pull requests targets a feature branch, this prefix helps as a filter 1. A detailed summary of what changed. Some hints: - 1. Keep it short but do not ommit important details. Usually we squash and merge, write what you would write as commit message of the squashed commit + 1. Keep it short but do not ommit important details. Usually we squash and merge, write what you would write as commit message of the squashed commit, later you can reuse what you wrote. 1. A link to each issue that is closed by the PR (e.g. Closes #123) or related (Relates #456) 1. Further information if necessary, maybe you want to share a screenshot, list open Todo's, describe your thought process, etc. 1. Optional: List of backport PR's, other related PR's 1. Label the PR, not all might apply: 1. `:ml` mandatory label, to be consistent with other repositories. - 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`. + 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`. 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version. 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature 1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption. 1. `discuss` If your PR suggests a change which you first like to discuss regarding it's functional changes before going deep into actual implementation details (e.g. you change a default) 1. `WIP` let's potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. 1. `review` Try to find reviewers for your change, github has some suggestions, however if unsure or you know the reviewer is very busy, `review` marks the PR as free to grab for review. Still, PR's are open to anyone to comment. - - ### Backport -There needs to be a link in the comments to the originating PR - for easy click through. +Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches e.g. `6.1`. If unsure, the versions to backport to, can be discussed on the PR. -We must create the backport PR at the same time as merging the original to master, although the backport may be merged later. +A backport starts right after merging the PR on `master`, please open backport PR's immediatly after merging, even if you plan to merge backports a little later. This helps to ensure that backports do not get lost. Rules for Backport PR's: -Backport PRs do not need to be reviewed, although you should if the backport becomes very different. +1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format` +1. Link to the originating PR in the description, you do not need to repeat the description as discussions should exclusively happen on the master PR. +1. Backports do not need a review, however if you had to do further changes (merge conflicts, compiler specialities) it's advised to let the adjustments get quickly reviewed +1. Label the PR with `>backport` and the version the backport, the full set of labels remain on the master PR. -Once we have it in place (soon hopefully), we will wait for the CI to succeed before merging the backport. -In the meantime, BuildMyBranch should still be used even for backports. -(The downstream impact of build failures cannot be underestimated. There has been recent pressure on the es team to do everything possible to make CI green again, so it's a highlighted topic in other teams right now.) - -Backport PRs will be labelled with a new label - `backport`. -(Note - if we decide to create a backport tool, then would love to move across labels e.g. bug, ehnancement) - -Originating PR will be labelled with the full set, as current - `v6.x` `x7.0.0` `bug` `:ml` `analytics` - -Backport check boxes in the originating PR or issue are a great idea, but can be left to personal choice. Because the originating PR/issue may get closed before the backports, I worry they will not get reviewed. +Although it might look simple, always wait for CI to confirm that changes are sound to avoid unnecessary build breaks. From ba5842ad9145823310216326c0cddc972ee09dae Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Mon, 28 May 2018 10:12:15 +0200 Subject: [PATCH 4/7] Update CONTRIBUTING.md --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fcd4ae3b6b..3a96f145a2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,9 +93,9 @@ The followings lists some guide lines when authoring pull requests. Note: Try to ### Backport -Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches e.g. `6.1`. If unsure, the versions to backport to, can be discussed on the PR. +Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches e.g. `6.1`. If unsure about the versions to backport to, it can be discussed on the master PR. -A backport starts right after merging the PR on `master`, please open backport PR's immediatly after merging, even if you plan to merge backports a little later. This helps to ensure that backports do not get lost. Rules for Backport PR's: +A backport starts right after merging the PR on `master`, please open backport PR's immediatly after merging to `master`, even if you intend to merge backports a little later (e.g. to wait/analyse QA results). This helps to ensure that backports do not get lost. Rules for Backport PR's: 1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format` 1. Link to the originating PR in the description, you do not need to repeat the description as discussions should exclusively happen on the master PR. From 1e1986c3ef5ac85514d23cad9a36b7ae3b6b07d4 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 29 May 2018 13:51:09 +0200 Subject: [PATCH 5/7] Update CONTRIBUTING.md address review comments --- CONTRIBUTING.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3a96f145a2..73d6d6e8ba 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,11 +69,11 @@ Please adhere to the general guideline that you should never force push to a pub Every change made to ml-cpp must be held to a high standard, Pull Requests are equally important as they document changes and decissions that have been made. `You Know, for Search` - a descriptive and relevant summary of the change helps people to find your PR later on. -The followings lists some guide lines when authoring pull requests. Note: Try to follow this guideline as close as possible but do not feel bad if you are unsure, team members can help you. +The followings lists some guidelines when authoring pull requests. Note, try to follow this guideline as close as possible but do not feel bad if you are unsure, team members can help you. 1. PR title summarizes the change, short and descriptive. 1. Use prefixes for quick categorization: - 1. [X.Y] Branch label if backport PR, if ommited the PR only applies to master. Note: Backport PR's are done after master PR, further explaination can be found below [#backport] + 1. [X.Y] Branch label if backport PR, if omited the PR only applies to master. Note, backport PR's are done after master PR, further explanation can be found below [#backport] 1. [ML] mandatory prefix, to be consistent with other repositories 1. [FEATURE] If your pull requests targets a feature branch, this prefix helps as a filter 1. A detailed summary of what changed. Some hints: @@ -86,20 +86,20 @@ The followings lists some guide lines when authoring pull requests. Note: Try to 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`. 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version. 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature - 1. `affects-results` If the PR is expected to have an affect on our QA test suite, that is any change that affects scoring but can also be any change that affects memory consumption. - 1. `discuss` If your PR suggests a change which you first like to discuss regarding it's functional changes before going deep into actual implementation details (e.g. you change a default) - 1. `WIP` let's potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. + 1. `affects-results` If the PR is expected to have a significant impact on results (e.g. our QA test suite). E.g. changes of scoring but also changes of memory consumption can indirectly lead to significant result changes. + 1. `discuss` If your PR suggests a change for which you'd first like to discuss regarding its functional changes before going deep into actual implementation details (e.g. you change a default) + 1. `WIP` lets potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. 1. `review` Try to find reviewers for your change, github has some suggestions, however if unsure or you know the reviewer is very busy, `review` marks the PR as free to grab for review. Still, PR's are open to anyone to comment. ### Backport -Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches e.g. `6.1`. If unsure about the versions to backport to, it can be discussed on the master PR. +Any development usually starts with an implementation on `master`, therefore any PR starts with a PR against `master`. Depending on the type we then decide about backport branches, features usually get backported to the active release branch, e.g. `6.x`, bugfixes might get backported to concrete release branches, e.g. `6.1`. If unsure about the versions to backport to, it can be discussed on the master PR. -A backport starts right after merging the PR on `master`, please open backport PR's immediatly after merging to `master`, even if you intend to merge backports a little later (e.g. to wait/analyse QA results). This helps to ensure that backports do not get lost. Rules for Backport PR's: +A backport starts right after merging the PR on `master`, please open backport PR's immediately after merging to `master`, even if you intend to merge backports a little later (e.g. to wait/analyse QA results). This helps to ensure that backports do not get lost. Rules for Backport PR's: -1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format` +1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format`. 1. Link to the originating PR in the description, you do not need to repeat the description as discussions should exclusively happen on the master PR. -1. Backports do not need a review, however if you had to do further changes (merge conflicts, compiler specialities) it's advised to let the adjustments get quickly reviewed -1. Label the PR with `>backport` and the version the backport, the full set of labels remain on the master PR. +1. Backports do not need a review, however if you had to do further changes (merge conflicts, compiler specialities) it's advised to let the adjustments get quickly reviewed. +1. Label the PR with `>backport` and the corresponding version of this backport, the full set of labels remain on the master PR. If the master PR is classified as `non-issue` also add the `non-issue` label due to CI checks. Although it might look simple, always wait for CI to confirm that changes are sound to avoid unnecessary build breaks. From e60bba5c160e15ebf8874a060c3a3d560dc81e2c Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 29 May 2018 13:53:21 +0200 Subject: [PATCH 6/7] Update CONTRIBUTING.md Improve affects-results description --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 73d6d6e8ba..b256b099ee 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -86,7 +86,7 @@ The followings lists some guidelines when authoring pull requests. Note, try to 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`. 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version. 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature - 1. `affects-results` If the PR is expected to have a significant impact on results (e.g. our QA test suite). E.g. changes of scoring but also changes of memory consumption can indirectly lead to significant result changes. + 1. `affects-results` If the PR is expected to have a significant impact on results(QA test suite), e.g. changes of scoring. Be aware that changes in terms of memory consumption can indirectly lead to significant result changes. 1. `discuss` If your PR suggests a change for which you'd first like to discuss regarding its functional changes before going deep into actual implementation details (e.g. you change a default) 1. `WIP` lets potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. 1. `review` Try to find reviewers for your change, github has some suggestions, however if unsure or you know the reviewer is very busy, `review` marks the PR as free to grab for review. Still, PR's are open to anyone to comment. From 5ca7a652721cf54284180b36f77150de3a5acb4a Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 29 May 2018 14:57:39 +0200 Subject: [PATCH 7/7] Update CONTRIBUTING.md fix the non-issue label --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b256b099ee..f823931f45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -85,7 +85,7 @@ The followings lists some guidelines when authoring pull requests. Note, try to 1. `:ml` mandatory label, to be consistent with other repositories. 1. `>type` Type of the PR, e.g. `>bug`, `>refactoring`, `>enhancement`, `>test`. 1. `vX.Y` Versions that PR should be applied to, a PR on master should always contain all backport versions as well, a backport PR should only have one corresponding version. - 1. `non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature + 1. `>non-issue` if the PR is not important for the changelog, e.g. a bugfix for an unreleased feature 1. `affects-results` If the PR is expected to have a significant impact on results(QA test suite), e.g. changes of scoring. Be aware that changes in terms of memory consumption can indirectly lead to significant result changes. 1. `discuss` If your PR suggests a change for which you'd first like to discuss regarding its functional changes before going deep into actual implementation details (e.g. you change a default) 1. `WIP` lets potential reviewers know, that you haven't completed the PR yet and you are still working on further changes, lets reviewers know to maybe wait a bit until you finalized the PR. @@ -100,6 +100,6 @@ A backport starts right after merging the PR on `master`, please open backport P 1. Prefix the title with `[X.Y]` and the mandatory `[ML]`, e.g. `[6.3][ML] Store expanding window bucket values in a compressed format`. 1. Link to the originating PR in the description, you do not need to repeat the description as discussions should exclusively happen on the master PR. 1. Backports do not need a review, however if you had to do further changes (merge conflicts, compiler specialities) it's advised to let the adjustments get quickly reviewed. -1. Label the PR with `>backport` and the corresponding version of this backport, the full set of labels remain on the master PR. If the master PR is classified as `non-issue` also add the `non-issue` label due to CI checks. +1. Label the PR with `>backport` and the corresponding version of this backport, the full set of labels remain on the master PR. If the master PR is classified as `>non-issue` also add the `>non-issue` label due to CI checks. Although it might look simple, always wait for CI to confirm that changes are sound to avoid unnecessary build breaks.