You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2021/04/17 12:31:19 UTC
[GitHub] [systemds] j143 opened a new pull request #1231: [DOC] add commit description guide
j143 opened a new pull request #1231:
URL: https://github.com/apache/systemds/pull/1231
Add description and tags usage. This PR can be the starting point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] corepointer commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
corepointer commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-821889383
One more question: [HOTFIX] - what is the hot branch? We practically work with one branch only, so that's always hot imho. In what way would it be different from a bugfix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] j143 commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
j143 commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-822858505
Hi @Baunsgaard - what do you think about this guide?
If everything's ok, can you provide an LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] asfgit closed pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1231:
URL: https://github.com/apache/systemds/pull/1231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] j143 commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
j143 commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-821815665
@corepointer - do you think this change will be helpful for us as a reference?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] phaniarnab commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
phaniarnab commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-821895625
I generally like this commit description guide. A crisp commit header and message have many advantages.
However, I would reduce the tag options to the bare minimum to remove confusion. Attaching a jira task to a commit is always the safest choice. MINOR is okay at times. I would not recommend using any other tags.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] corepointer commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
corepointer commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-821889127
> @corepointer - do you think this change will be helpful for us as a reference?
Yes, this could definitely be helpful. This is the first list of tags I see. Is the list complete? Where did you get it? Was there a discussion about the commit style that I missed? Can there be new tags? (e.g., [CLEANUP] ?)
Thank you for this PR! I think this is a good starting point for discussion where somebody with a longer history of SystemML/SystemDS can weigh in (maybe on the mailing list?)
Best regards, Mark
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on a change in pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#discussion_r616443807
##########
File path: CONTRIBUTING.md
##########
@@ -17,19 +17,17 @@ limitations under the License.
{% end comment %}
-->
-# Contributing to SystemDS
+## Contribution Guidelines and Standards for `SystemDS`
Review comment:
Main header should be with one '#'
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
+
+
+### Tags
+
+The tags can be used in combination to one another. These are the only tags available.
+
+* `[MINOR]`: Small changesets with additional functionality
+
+ > Examples:
+ >
+ > This commit makes small software updates with refactoring
+ >
+ > [`030fdab3`](https://github.com/apache/systemds/commit/030fdab3ebe6dedc3b4bb860e0ec5acfd9c38e5d) - `[MINOR] Added package to R dependency; updated Docker test image`
+ >
+ > This commit cleans up the redundant code for simplification
+ >
+ > [`f4fa5650`](https://github.com/apache/systemds/commit/f4fa565013de13270df05dd37610382ca80f7354) - `[MINOR][SYSTEMDS-43] Cleanup scale builtin function (readability)`
+ >
+
+* `[SYSTEMDS-#]`: A changeset which will a specific purpose such as a bug, Improvement,
+ New feature, etc. The tag value is found from [SystemDS jira issue tracker](https://issues.apache.org/jira/projects/SYSTEMDS/issues).
+
+* `[DOC]` also `[DOCS]`: Changes to the documentation
+
+* `[HOTFIX]`: Introduce a fix in the _hot_ running system.
Review comment:
i don't like the description here, it should say that it is commits that change behavior on already released versions, rather than using slang such as hot.
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
+
+
+### Tags
+
+The tags can be used in combination to one another. These are the only tags available.
+
+* `[MINOR]`: Small changesets with additional functionality
+
+ > Examples:
+ >
+ > This commit makes small software updates with refactoring
+ >
+ > [`030fdab3`](https://github.com/apache/systemds/commit/030fdab3ebe6dedc3b4bb860e0ec5acfd9c38e5d) - `[MINOR] Added package to R dependency; updated Docker test image`
+ >
+ > This commit cleans up the redundant code for simplification
+ >
+ > [`f4fa5650`](https://github.com/apache/systemds/commit/f4fa565013de13270df05dd37610382ca80f7354) - `[MINOR][SYSTEMDS-43] Cleanup scale builtin function (readability)`
+ >
+
+* `[SYSTEMDS-#]`: A changeset which will a specific purpose such as a bug, Improvement,
+ New feature, etc. The tag value is found from [SystemDS jira issue tracker](https://issues.apache.org/jira/projects/SYSTEMDS/issues).
+
+* `[DOC]` also `[DOCS]`: Changes to the documentation
+
+* `[HOTFIX]`: Introduce a fix in the _hot_ running system.
+
+ > Example:
+ >
+ > This commit fixes the corrupted language path
+ >
+ > [`87bc3584`](https://github.com/apache/systemds/commit/87bc3584db2148cf78b2d46418639e88ca27ec64) - `[HOTFIX] Fix validation of scalar-scalar binary min/max operations`
+ >
+
+### Commit description
+
+> A commit or PR description is a public record of **what** change is being made and **why**.
+
+#### Structure of the description
+
+##### First Line
+
+1. A summary of what the changeset.
+2. A complete sentence, crafted as though it was an order.
+ - an imperative sentence
+ - Writing the rest of the description as an imperative is optional.
+3. Follow by an empty line.
+
+##### Body
+
+It consists of the following.
+
+1. A brief description of the problem solved.
+2. Why this is the best approach?.
+3. Shortcomings to the approach, if any (important!).
+
+Additional info
+
+4. background information
+ - bug/issue/jira numbers
+ - benchmark/test results
Review comment:
Going into to much detail, you only need two spaces to make an sub itemize in markdown.
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
+
+
+### Tags
+
+The tags can be used in combination to one another. These are the only tags available.
+
+* `[MINOR]`: Small changesets with additional functionality
+
+ > Examples:
+ >
+ > This commit makes small software updates with refactoring
+ >
+ > [`030fdab3`](https://github.com/apache/systemds/commit/030fdab3ebe6dedc3b4bb860e0ec5acfd9c38e5d) - `[MINOR] Added package to R dependency; updated Docker test image`
+ >
+ > This commit cleans up the redundant code for simplification
+ >
+ > [`f4fa5650`](https://github.com/apache/systemds/commit/f4fa565013de13270df05dd37610382ca80f7354) - `[MINOR][SYSTEMDS-43] Cleanup scale builtin function (readability)`
+ >
+
+* `[SYSTEMDS-#]`: A changeset which will a specific purpose such as a bug, Improvement,
+ New feature, etc. The tag value is found from [SystemDS jira issue tracker](https://issues.apache.org/jira/projects/SYSTEMDS/issues).
+
+* `[DOC]` also `[DOCS]`: Changes to the documentation
+
+* `[HOTFIX]`: Introduce a fix in the _hot_ running system.
+
+ > Example:
+ >
+ > This commit fixes the corrupted language path
+ >
+ > [`87bc3584`](https://github.com/apache/systemds/commit/87bc3584db2148cf78b2d46418639e88ca27ec64) - `[HOTFIX] Fix validation of scalar-scalar binary min/max operations`
+ >
+
+### Commit description
+
+> A commit or PR description is a public record of **what** change is being made and **why**.
+
+#### Structure of the description
+
+##### First Line
+
+1. A summary of what the changeset.
+2. A complete sentence, crafted as though it was an order.
+ - an imperative sentence
+ - Writing the rest of the description as an imperative is optional.
+3. Follow by an empty line.
+
+##### Body
+
+It consists of the following.
+
+1. A brief description of the problem solved.
+2. Why this is the best approach?.
+3. Shortcomings to the approach, if any (important!).
+
+Additional info
+
+4. background information
+ - bug/issue/jira numbers
+ - benchmark/test results
+ - links to design documents
+ - code attributions
+5. Include enough context for
+ - reviewers
+ - future readers to understand the Changes.
+6. Add PR number, like `Closes #1000`.
+
+The following is a commit description with all the points mentioned.
+
+[`1abe9cb`](https://github.com/apache/systemds/commit/1abe9cb79d8001992f1c79ba5e638e6b423a1382)
+
+```
+[SYSTEMDS-418] Performance improvements lineage reuse probing/spilling
+This patch makes some minor performance improvements to the lineage
+reuse probing and cache put operations. Specifically, we now avoid
+unnecessary lineage hashing and comparisons by using lists instead of
+hash maps, move the time computations into the reuse path (to not affect
+the code path without lineage reuse), avoid unnecessary branching, and
+materialize the score of cache entries to avoid repeated computation
+for the log N comparisons per add/remove/constaints operation.
+For 100K iterations and ~40 ops per iteration, lineage tracing w/ reuse
+improved from 41.9s to 38.8s (pure lineage tracing: 27.9s).
+```
+
+#### Good commit description
+
+##### Functionality change
+
+[`1101533`](https://github.com/apache/systemds/commit/1101533fd1b2be4e475a18052dbb4bc930bb05d9)
+
+```
+[SYSTEMDS-2603] New hybrid approach for lineage deduplication
+This patch makes a major refactoring of the lineage deduplication
+framework. This changes the design of tracing all the
+distinct paths in a loop-body before the first iteration, to trace
+during execution. The number of distinct paths grows exponentially
+with the number of control flow statements. Tracing all the paths
+in advance can be a huge waste and overhead.
+We now trace an iteration during execution. We count the number of
+distinct paths before the iterations start, and we stop tracing
+once all the paths are traced. Tracing during execution fits
+very well with our multi-level reuse infrastructure.
+Refer to JIRA for detailed discussions.
+```
+
+
+##### Refactoring
+
+[`e581b5a`](https://github.com/apache/systemds/commit/e581b5a6248b56a70e18ffe6ba699e8142a2d679)
+
+```
+[SYSTEMDS-2575] Fix eval function calls (incorrect pinning of inputs)
+This patch fixes an issue of indirect eval function calls where wrong
+input variable names led to missing pinning of inputs and thus too eager
+cleanup of these variables (which causes crashes if the inputs are used
+in other operations of the eval call).
+The fix is simple. We avoid such inconsistent construction and
+invocation of fcall instructions by using a narrower interface and
+constructing the materialized names internally in the fcall.
+```
+
+##### Small Changeset still needs some context.
+
+Example [`7af2ae0`](https://github.com/apache/systemds/commit/7af2ae04f28ddcb36158719a25a7fa34b22d3266)
Review comment:
Only place where you write example
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
+
+
+### Tags
+
+The tags can be used in combination to one another. These are the only tags available.
+
+* `[MINOR]`: Small changesets with additional functionality
+
+ > Examples:
+ >
+ > This commit makes small software updates with refactoring
+ >
+ > [`030fdab3`](https://github.com/apache/systemds/commit/030fdab3ebe6dedc3b4bb860e0ec5acfd9c38e5d) - `[MINOR] Added package to R dependency; updated Docker test image`
+ >
+ > This commit cleans up the redundant code for simplification
+ >
+ > [`f4fa5650`](https://github.com/apache/systemds/commit/f4fa565013de13270df05dd37610382ca80f7354) - `[MINOR][SYSTEMDS-43] Cleanup scale builtin function (readability)`
+ >
+
+* `[SYSTEMDS-#]`: A changeset which will a specific purpose such as a bug, Improvement,
+ New feature, etc. The tag value is found from [SystemDS jira issue tracker](https://issues.apache.org/jira/projects/SYSTEMDS/issues).
+
+* `[DOC]` also `[DOCS]`: Changes to the documentation
+
+* `[HOTFIX]`: Introduce a fix in the _hot_ running system.
+
+ > Example:
+ >
+ > This commit fixes the corrupted language path
+ >
+ > [`87bc3584`](https://github.com/apache/systemds/commit/87bc3584db2148cf78b2d46418639e88ca27ec64) - `[HOTFIX] Fix validation of scalar-scalar binary min/max operations`
+ >
+
+### Commit description
+
+> A commit or PR description is a public record of **what** change is being made and **why**.
+
+#### Structure of the description
+
+##### First Line
+
+1. A summary of what the changeset.
+2. A complete sentence, crafted as though it was an order.
+ - an imperative sentence
+ - Writing the rest of the description as an imperative is optional.
+3. Follow by an empty line.
+
+##### Body
+
+It consists of the following.
+
+1. A brief description of the problem solved.
+2. Why this is the best approach?.
+3. Shortcomings to the approach, if any (important!).
+
+Additional info
+
+4. background information
+ - bug/issue/jira numbers
+ - benchmark/test results
+ - links to design documents
+ - code attributions
+5. Include enough context for
+ - reviewers
+ - future readers to understand the Changes.
+6. Add PR number, like `Closes #1000`.
+
+The following is a commit description with all the points mentioned.
+
+[`1abe9cb`](https://github.com/apache/systemds/commit/1abe9cb79d8001992f1c79ba5e638e6b423a1382)
+
+```
Review comment:
It is good style to specify what type of code block you have in markdown, here in this instance it is txt
##########
File path: CONTRIBUTING.md
##########
@@ -17,19 +17,17 @@ limitations under the License.
{% end comment %}
-->
-# Contributing to SystemDS
+## Contribution Guidelines and Standards for `SystemDS`
-Thanks for taking the time to contribute to SystemDS!
+Thank you for contributing to SystemDS. :smile:
Review comment:
:+1:
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
Review comment:
do not force push
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] j143 commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
j143 commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-823280790
Thanks everyone here for the comments. 😸
Sorry, for pinging individually. Style changes such as this one needs to have sufficient consensus.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] j143 commented on pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
j143 commented on pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#issuecomment-821890273
I have been with SystemML/DS team from 2016! 💖
> Is the list complete? Where did you get it? Was there a discussion about the commit style that I missed?
1. I have analyzed the complete commit history (from 2010).
2. And have listened to most of the discussion on the list that of the SystemML (previous) and the present team.
> Can there be new tags? (e.g., [CLEANUP] ?)
This is the complete list of the tags. But, we can adjust or create new ones with functionality.
> what is the hot branch?
There my intention is in the release context. Needs rewording. 😕
> Hotfix vs bugfix
@mboehm7 - can you help me out here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] j143 commented on a change in pull request #1231: [DOC] add commit description guide
Posted by GitBox <gi...@apache.org>.
j143 commented on a change in pull request #1231:
URL: https://github.com/apache/systemds/pull/1231#discussion_r616663331
##########
File path: CONTRIBUTING.md
##########
@@ -54,7 +52,151 @@ We have provided at profile for java located in [Codestyle File ./docs/CodeStyle
- [IntelliJ](https://imagej.net/Eclipse_code_style_profiles_and_IntelliJ)
- [Visual Studio Code](https://stackoverflow.com/questions/46030629/need-to-import-eclipse-java-formatter-profile-in-visual-studio-code)
-#### License
+## Commit Style
+
+SystemDS project has linear history throughout. Rebase cleanly and verify that the commit-SHA's
+of the Apache Repo are not altered.
+A general guideline is never to change a commit inside the systemds official repo, which is
+the standard practice. If you have accidentally committed a functionality or tag, let others know.
+And create a new commit to revert the functionality of the previous commit but **not force push**.
+
+
+### Tags
+
+The tags can be used in combination to one another. These are the only tags available.
+
+* `[MINOR]`: Small changesets with additional functionality
+
+ > Examples:
+ >
+ > This commit makes small software updates with refactoring
+ >
+ > [`030fdab3`](https://github.com/apache/systemds/commit/030fdab3ebe6dedc3b4bb860e0ec5acfd9c38e5d) - `[MINOR] Added package to R dependency; updated Docker test image`
+ >
+ > This commit cleans up the redundant code for simplification
+ >
+ > [`f4fa5650`](https://github.com/apache/systemds/commit/f4fa565013de13270df05dd37610382ca80f7354) - `[MINOR][SYSTEMDS-43] Cleanup scale builtin function (readability)`
+ >
+
+* `[SYSTEMDS-#]`: A changeset which will a specific purpose such as a bug, Improvement,
+ New feature, etc. The tag value is found from [SystemDS jira issue tracker](https://issues.apache.org/jira/projects/SYSTEMDS/issues).
+
+* `[DOC]` also `[DOCS]`: Changes to the documentation
+
+* `[HOTFIX]`: Introduce a fix in the _hot_ running system.
+
+ > Example:
+ >
+ > This commit fixes the corrupted language path
+ >
+ > [`87bc3584`](https://github.com/apache/systemds/commit/87bc3584db2148cf78b2d46418639e88ca27ec64) - `[HOTFIX] Fix validation of scalar-scalar binary min/max operations`
+ >
+
+### Commit description
+
+> A commit or PR description is a public record of **what** change is being made and **why**.
+
+#### Structure of the description
+
+##### First Line
+
+1. A summary of what the changeset.
+2. A complete sentence, crafted as though it was an order.
+ - an imperative sentence
+ - Writing the rest of the description as an imperative is optional.
+3. Follow by an empty line.
+
+##### Body
+
+It consists of the following.
+
+1. A brief description of the problem solved.
+2. Why this is the best approach?.
+3. Shortcomings to the approach, if any (important!).
+
+Additional info
+
+4. background information
+ - bug/issue/jira numbers
+ - benchmark/test results
Review comment:
Aligned to the main points, for aesthetic reason in the editor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org