[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request #16699: Mixed data type binary ops
haojin2 commented on a change in pull request #16699: Mixed data type binary ops URL: https://github.com/apache/incubator-mxnet/pull/16699#discussion_r342324211 ## File path: src/operator/mshadow_op.h ## @@ -194,6 +194,100 @@ MXNET_BINARY_MATH_OP_NC(right, b); MXNET_BINARY_MATH_OP_NC(mul, a * b); +#ifndef _WIN32 Review comment: It was due to the `C1002: out of heap space` error we've encountered many times. We're not generating more kernels(code) on windows to prevent hitting that error on windows machines. If you still think windows is excluded I would only think you've not given the code changes a complete look: 1. There's also some parts that we have `#ifdef _WIN32` such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 2. We also have parts that have `#else` blocks such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 This is indeed a workaround for an issue that we could not solve on our own. I've also tried with upgrading vs compiler locally and it does not get this issue out of our way so that's why we have different impls for this same feature, otherwise we only have to drop this new feature for windows users. It's good to be eager to learn, but IMHO blocking a PR without a complete look and a very solid reason is not a good (nor polite) way for demonstrating your eagerness. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request #16699: Mixed data type binary ops
haojin2 commented on a change in pull request #16699: Mixed data type binary ops URL: https://github.com/apache/incubator-mxnet/pull/16699#discussion_r342324211 ## File path: src/operator/mshadow_op.h ## @@ -194,6 +194,100 @@ MXNET_BINARY_MATH_OP_NC(right, b); MXNET_BINARY_MATH_OP_NC(mul, a * b); +#ifndef _WIN32 Review comment: It was due to the `C1002: out of heap space` error we've encountered many times. We're not generating more kernels(code) on windows to prevent hitting that error on windows machines. If you still think windows is excluded I would only think you've not given the code changes a complete look: 1. There's also some parts that we have `#ifdef _WIN32` such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 2. We also have parts that have `#else` blocks such as: https://github.com/apache/incubator-mxnet/pull/16699/files#diff-c383124e9cb87f51ac456a96b799615aR73 This is indeed a workaround for an issue that we could not solve on our own. I've also tried with upgrading vs compiler locally and it does not get this issue out of our way so that's why we have different impls for this same feature, otherwise we only have to drop this new feature for windows users. It's good to be eager to learn, but IMHO blocking a PR without a complete look and a very solid reason is not a good (nor polite) way for demonstrating your eagerness. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request #16699: Mixed data type binary ops
haojin2 commented on a change in pull request #16699: Mixed data type binary ops URL: https://github.com/apache/incubator-mxnet/pull/16699#discussion_r342316290 ## File path: src/operator/mshadow_op.h ## @@ -194,6 +194,100 @@ MXNET_BINARY_MATH_OP_NC(right, b); MXNET_BINARY_MATH_OP_NC(mul, a * b); +#ifndef _WIN32 Review comment: It's supported with a different implementation due to limitations of the windows vs compiler. The fact that the corresponding unit tests are not discriminative against windows machines and they passed both windows cpu and gpu checks means this feature is also supported on windows. Please do make sure you have some grasp of the big picture of a PR before you block one. 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: us...@infra.apache.org With regards, Apache Git Services
