You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/05/21 16:09:44 UTC

[GitHub] [incubator-mxnet] yajiedesign opened a new pull request #18379: change _Pragma to #pragma

yajiedesign opened a new pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379


   ## Description ##
   change _Pragma to #pragma
   _Pragma not work with msvc
   
   ## Checklist ##
   ### Essentials ###
   - [x] Changes are complete (i.e. I finished coding on this PR)
   ### Changes ###
   ## Comments ##
   
   


----------------------------------------------------------------
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] [incubator-mxnet] zhreshold merged pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
zhreshold merged pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379


   


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632181662


   https://github.com/apache/incubator-mxnet/issues/18374


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632961357


   @leezu Because CUDA. The old version of CUDA does not support the new version of msvc.
   CI is only one CUDA version. but CD requires multiple versions of CUDA. cannot upgrade MSVC.


----------------------------------------------------------------
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] [incubator-mxnet] zhreshold commented on a change in pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
zhreshold commented on a change in pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#discussion_r428841505



##########
File path: src/io/batchify.cc
##########
@@ -150,7 +150,7 @@ class StackBatchify : public BatchifyFunction {
         }
         int sbs = static_cast<int>(bs);
         MSHADOW_TYPE_SWITCH_WITH_BOOL(dtype, DType, {
-          _Pragma("omp parallel for num_threads(bs)")
+          #pragma omp parallel for num_threads(bs)

Review comment:
       use `__pragma` for msvc if it does not support `_Pragma`




----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-634370280


   @zhreshold


----------------------------------------------------------------
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] [incubator-mxnet] leezu commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632974522


   MXNet 2 will be released after Cuda 11 release, so there is no Cuda 9 support in MXNet 2 currently


----------------------------------------------------------------
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] [incubator-mxnet] wkcn commented on a change in pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#discussion_r428988397



##########
File path: src/io/batchify.cc
##########
@@ -150,7 +150,7 @@ class StackBatchify : public BatchifyFunction {
         }
         int sbs = static_cast<int>(bs);
         MSHADOW_TYPE_SWITCH_WITH_BOOL(dtype, DType, {
-          _Pragma("omp parallel for num_threads(bs)")
+          #pragma omp parallel for num_threads(bs)

Review comment:
       We can unify the macro like
   ```c++
   #ifndef _MSC_VER
   #define __pragma(id) _Pragma(#id)
   #endif
   ```




----------------------------------------------------------------
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] [incubator-mxnet] leezu commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632964034


   (Soon Cuda 11 will be released and we'll drop Cuda 9 support in any case) 


----------------------------------------------------------------
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] [incubator-mxnet] leezu commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632963894


   Thanks for clarifying. In that case, should we either drop the old Cuda versions for MXNet 2 or switch the CI to an older MSVC version? What do you think? 


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-633137800


   @mxnet-bot run ci [macosx-x86_64]


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632430692


   I'll try upgrading vs


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632461418


   Cannot be resolved by upgrade vs.


----------------------------------------------------------------
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] [incubator-mxnet] leezu removed a comment on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu removed a comment on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632974522


   MXNet 2 will be released after Cuda 11 release, so there is no Cuda 9 support in MXNet 2 currently


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632641019


   @mxnet-bot run ci [windows-cpu]


----------------------------------------------------------------
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] [incubator-mxnet] zhreshold commented on a change in pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
zhreshold commented on a change in pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#discussion_r429381407



##########
File path: src/io/batchify.cc
##########
@@ -37,6 +37,17 @@
 
 namespace mxnet {
 namespace io {
+
+#ifdef _MSC_VER
+  #if _MSC_VER < 1925
+    #define omp_parallel __pragma(omp parallel for num_threads(bs))

Review comment:
       Can you make `bs` argument of the macro?




----------------------------------------------------------------
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] [incubator-mxnet] leezu commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632996894


   Yes, that's an option. I'm not convinced though that only fixing this current incompatibility without introducing any mechanism of preventing future problems is helpful. 
   So if you think we need to support MSVC 2017, then let's also turn it on in the CI. If not, then let's just use MSVC 2019 on the CD. But supporting MSVC 2017 without having any CI checks would just cause unnecessary problems down the road (with the CD breaking often) 


----------------------------------------------------------------
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] [incubator-mxnet] zhreshold commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
zhreshold commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632274216


   `#pragma` won't work in a macro, what's the version of msvc? At least 2019 supports `_Pragma`


----------------------------------------------------------------
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] [incubator-mxnet] mxnet-bot commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632181495


   Hey @yajiedesign , Thanks for submitting the PR 
   All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands: 
   - To trigger all jobs: @mxnet-bot run ci [all] 
   - To trigger specific jobs: @mxnet-bot run ci [job1, job2] 
   *** 
   **CI supported jobs**: [windows-gpu, edge, clang, miscellaneous, windows-cpu, centos-cpu, centos-gpu, unix-cpu, website, sanity, unix-gpu]
   *** 
   _Note_: 
    Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin. 
   All CI tests must pass before the PR can be merged. 
   


----------------------------------------------------------------
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] [incubator-mxnet] yajiedesign commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
yajiedesign commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632965410


   Keep the MSVC of CI compatible with the minimum supported CUDA version.
   Then upgrade MSVC when drop CUDA version support.


----------------------------------------------------------------
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] [incubator-mxnet] mxnet-bot commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-633137811


   None of the jobs entered are supported. 
   Jobs entered by user: [macosx-x86_64]
   CI supported Jobs: [edge, windows-cpu, centos-cpu, website, windows-gpu, miscellaneous, centos-gpu, unix-cpu, clang, unix-gpu, sanity]
   


----------------------------------------------------------------
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] [incubator-mxnet] leezu commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-634416400


   @yajiedesign please clarify how you plan to address the CI CD discrepancy?


----------------------------------------------------------------
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] [incubator-mxnet] mxnet-bot commented on pull request #18379: change _Pragma to #pragma

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18379:
URL: https://github.com/apache/incubator-mxnet/pull/18379#issuecomment-632641079


   Jenkins CI successfully triggered : [windows-cpu]


----------------------------------------------------------------
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