[GitHub] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-446253481 @larroy This PR just fixes a broken existing feature. Why do we need cython? We may have some performance gain. Here is the copy of a quick benchmark result I reported before. For a quick benchmark I have run `example/gluon/image_classification.py` with `--model=resnet50_v1 --dataset=cifar10` on a machine with two Xeon E5-2680 and 1080ti. The mean time consumption per epoch was 72.2s with cython and 78.6s without cython. So the the running time with cython is about 8% shorter. With `--model=resnet101_v1`, the gain was about 10% (135.3s vs 150.2s). For cmake, I added `BUILD_CYTHON_MODULES` option. 'GPU: CMake' environment builds the cython modules via cmake but no unit test runs with the artifact from the build. I left a comment about the llvm openmp issue at the unit test 'Python3: GPU'. https://github.com/apache/incubator-mxnet/blob/adce15e25afdf3a47f5dc26bda9db7371aa4882b/ci/docker/runtime_functions.sh#L665-L682 https://github.com/apache/incubator-mxnet/blob/10d817cea15ec0b3253f3b78a30052f764931026/ci/jenkins/Jenkins_steps.groovy#L599-L612 This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-440091940 Any discussion on the last change? Or can we get this PR merged? This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-436900249 #12160 looks like delayed too much. The issue is that the cmake build with llvm openmp causes a segfault in the CI environment. The segfault also occurs without cython. So the issue is not directly related to this PR. Of course, it would be better if we could test the cython build with cmake. However, in current situation, I think that we need to separate this PR with the cmake build issue. I removed the testing of the cython modules with cmake. 'CPU: Openblas' and 'GPU: CUDA9.1+cuDNN7' build cython module with make. 'Python2: CPU' and 'Python3: GPU' runs unit tests with the cython modules. 'GPU: CMake' still builds cython modules with cmake but there is no test with them. @marcoabreu @larroy @lebeg This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-428091496 @vrakesh I'm still waiting on #12160. I'll update this PR to resolve the conflict and make sure that it passes the tests after #12160 is addressed. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-412778840 It looks like that the segmentation fault from the cmake build is related to #11417. [Here](https://github.com/apache/incubator-mxnet/issues/11417#issuecomment-412777877) is some details related to this PR. I'm waiting for the issue to be addressed. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-410495113 - We need to build cython modules for both of python2 and python3. Once the two builds are successful, I think we don't strictly need to run the tests for both. However, I think maintain the two tests in combinations with make/cmake would be a good idea. - Currently "Python2 CPU" and "Python3 GPU" don't use cmake. One solution would be modifying "Python3 GPU" to run the test based on ''GPU: CMake'' build. So "CPU: Openblas" and "GPU: CMake" build the cython modules, and we have two unit test environments with cython: "Python2 CPU" = (make, cpu, python2) and "Python3 GPU"= (cmake, gpu, python3). I'll try this modification. - I set MXNET_ENABLE_CYTHON=0 explicitly for all other python test environments. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-409782946 @apeforest My preference is the current way of using two env variables. I think it's not so confusing. My confusion was because there was no clear documentation on the variables. So this PR adds the documentation. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-409491960 For a quick benchmark I have run `example/gluon/image_classification.py` with `--model=resnet50_v1 --dataset=cifar10` on a machine with two Xeon E5-2680 and 1080ti. The mean time consumption per epoch was 72.2s with cython and 78.6s without cython. So the the running time with cython is about 8% shorter than the running time without cython. With `--model=resnet101_v1`, the gain was about 10% (135.3s vs 150.2s). This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-409469161 Would you please merge this PR if there is no problem? This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-398577395 I removed the use of the cython modules from the Cent OS environments as @marcoabreu requested. Now "CPU: Openblas" and "GPU: Cuda 9.1" enviroments build the cython modules, and "Python 2: GPU" and "Python 3: CPU" use the cython modules to run the tests. They also run `check_cython` in the above comment to see that the cython modules are actually used. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-398307755 A way to check whether or not cython is actually used at runtime is seeing `mx.nd._internal.NDArrayBase`. It must be `mxnet._cy3.ndarray.NDArrayBase` if cython is used and `mxnet._ctypes.ndarray.NDArrayBase` otherwise. So, if it is desirable, we could run a test like this before the unit tests run. ```bash if [ "$(echo -e 'import mxnet as mx\nprint(mx.nd._internal.NDArrayBase)' | python)" != "" ]; then echo "Cython is not used." exit 1 fi ``` This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397175348 ```bash make make cython cd python # `pip install -e .` or export PYTHONPATH=my_installation_dir python setup.py install --with-cython --prefix=my_installation_dir # run tests ... ``` I think that this is the typical way to build and run the tests. cmake build would be similar. Setting cxx/nvcc compiler flags is not needed. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397852430 I added the cython build to Python3 CentOS 7 CPU and GPU environments. I have made an intentional bug in the cython module and confirmed that it actually causes [the failures of the unit tests](http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-10951/16/pipeline/739). I think that now this PR is completed. Please review the code. Here is the summary of the changes. 1. A bug in the `setup.py` which causes problems with the recent cython 0.28 (#10295, #10738) is fixed. 1. The cython module for the ndarray has caused segmentation faults since the introduction of the sparse array. This is fixed. 1. Now the `.so` files generated by cython has `rpath` to find `libmxnet.so`, so the deployment is easier. 1. A new makefile variable `PYTHON` to set the python executable is added (`make/config.mk`). The cython build target needs to run python but some environments have nonstandard executable name such as `python3.6`. 1. Documentation for the environment variables `MXNET_ENABLE_CYTHON` and `MXNET_ENFORCE_CYTHON` is added. 1. Documentation for the optional cython build is added to the install guide. 1. Python3 CentOS 7 CPU and Python3 CentOS 7 GPU environments in the CI now use the cython modules. `make cython` generates two `.so` files in `mxnet/_cy3` (or `_cy2` with python 2). The exact names of the files depend on the environment. They don't need a special care for installation. It is enough just to be there. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397852430 I added the cython build to Python3 CentOS 7 CPU and GPU environments. I have made an intentional bug in the cython module and confirmed that it actually causes [the failures of the unit tests](http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-10951/16/pipeline/739). I think that now this PR is completed. Please review the code. Here is the summary of the changes. 1. A bug in the `setup.py` which causes problems with the recent cython 0.28 (#10295, #10738) is fixed. 1. The cython module for the ndarray has caused segmentation faults since the introduction of the sparse array. This is fixed. 1. Now the compiled cython module has `rpath` to find `libmxnet.so`, so the deployment is easier. 1. A new makefile variable `PYTHON` to set the python executable is added (`make/config.mk`). The cython build target needs to run python but some environments have nonstandard executable name such as `python3.6`. 1. Documentation for the environment variables `MXNET_ENABLE_CYTHON` and `MXNET_ENFORCE_CYTHON` is added. 1. Documentation for the optional cython build is added to the install guide. 1. Python3 CentOS 7 CPU and Python3 CentOS 7 GPU environments in the CI now use the cython modules. `make cython` generates two `.so` files in `mxnet/_cy3` (or `_cy2` with python 2). The exact names of the files depend on the environment. They don't need a special care for installation. It is enough just to be there. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397852430 I added the cython build to Python3 CentOS 7 CPU and GPU environments. I have made an intentional bug in the cython module and confirmed that it actually causes [the failures of the tests](http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-10951/16/pipeline/739). I think that now this PR is completed. Please review the code. Here is the summary of the changes. 1. A bug in the `setup.py` which causes problems with the recent cython 0.28 (#10295, #10738) is fixed. 1. The cython module for the ndarray has caused segmentation faults since the introduction of the sparse array. This is fixed. 1. Now the compiled cython module has `rpath` to find `libmxnet.so`, so the deployment is easier. 1. A new makefile variable `PYTHON` to set the python executable is added (`make/config.mk`). The cython build target needs to run python but some environments have nonstandard executable name such as `python3.6`. 1. Documentation for the environment variables `MXNET_ENABLE_CYTHON` and `MXNET_ENFORCE_CYTHON` is added. 1. Documentation for the optional cython build is added to the install guide. 1. Python3 CentOS 7 CPU and Python3 CentOS 7 GPU environments in the CI now use the cython modules. `make cython` generates two `.so` files in `mxnet/_cy3` (or `_cy2` with python 2). The exact names of the files depend on the environment. They don't need a special care for installation. It is enough just to be there. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397175348 ```bash make cd python make cython # pip install or export PYTHONPATH=my_installation_dir python setup.py install --with-cython --prefix=my_installation_dir cd ../tests/python # run tests ... ``` I think that this is the typical way to build and run the tests. cmake build would be similar. Setting cxx/nvcc compiler flags is not needed. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397175348 ```bash make cd python make cython # pip install or export PYTHONPATH=my_installation_dir python setup.py install --with-cython --prefix=my_installation_dir cd ../tests/python # run tests ... ``` I think that this is the typical way to build and run the tests. cmake build would be similar. Setting cxx/nvcc compiler flags is not required. This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397172463 @marcoabreu Thank you. First, I'm just not sure what is the best way to add a cython build to CI. There are already many environments. Would you recommend some existing (linux) environments to add cython build? Or is it a good idea to add new environments for cython build? Second, I'm not familiar with Jenkins and docker. It looks like that I need to edit `Jenkinsfile` and files under `docker` dir. Are there any other files that I need to check? This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397172463 @marcoabreu Thank you. First, I'm just not sure what is the best way to add a cython build to CI. There are already many environments. Would you recommend some existing environments to add cython build? Or is it a good idea to add new environments for cython build? Second, I'm not familiar with Jenkins and docker. It looks like that I need to edit `Jenkinsfile` and files under `docker` dir. Are there any other files that I need to check? This is an automated message from the Apache Git Service. To respond to the message, please log on 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] asitstands commented on issue #10951: [MXNET-545] Fix broken cython build
asitstands commented on issue #10951: [MXNET-545] Fix broken cython build URL: https://github.com/apache/incubator-mxnet/pull/10951#issuecomment-397151382 @piiswrong @szha @marcoabreu Would you review this PR? This is an automated message from the Apache Git Service. To respond to the message, please log on 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
