[MXNET-545] Fix broken cython build #10951

asitstands · 2018-05-15T14:16:44Z

Description

Fix broken cython module for ndarray and also ensure the compatibility with the recent version (0.28) of cython (#10295, #10738). See this comment for the details of the changes.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

szha · 2018-05-21T21:41:17Z

How is this tested?

asitstands · 2018-05-22T04:16:25Z

@szha I just checked that .cpp files are generated at python/mxnet/cython and .so files at python/build/lib.linux-x86_64-3.6/mxnet/_cy3. All unit tests are passed in my environment. I'm not sure CI has cython build. Are there any other checks do you recommend?

szha · 2018-05-22T04:20:43Z

Thanks. @marcoabreu do you have recommendation on how to integrate the tests for cython (e.g. whether to integrate into existing environments or to add a new environment)?

asitstands · 2018-05-22T15:22:19Z

This PR does not work as expected. I made a mistake to set MXNET_ENABLE_CYTHON instead of MXNET_ENFORCE_CYTHON when I tested. An import error occurs if a python script using mxnet runs with MXNET_ENFORCE_CYTHON=1.

Traceback (most recent call last):
  File "/pds/pds131/asitdepends/MXNet/usr/lib/python3.6/site-packages/mxnet-1.2.0-py3.6-linux-x86_64.egg/mxnet/ndarray/_internal.py", line 30, in <module>
    from .._cy3.ndarray import NDArrayBase, CachedOp
ImportError: /pds/pds131/asitdepends/MXNet/usr/lib/python3.6/site-packages/mxnet-1.2.0-py3.6-linux-x86_64.egg/mxnet/_cy3/ndarray.cpython-36m-x86_64-linux-gnu.so: undefined symbol: NNGetLastError

The .so files from the .pyx files are created but the linker does not find libmxnet.so. ~~I guess that this line is the source of the problem. However the cython compilation itself fails without that line. The compile error is~~

~~error: each element of 'ext_modules' option must be an Extension instance or 2-tuple~~

~~as reported in #10295.~~

asitstands · 2018-05-26T15:36:07Z

Now, it passed all tests under tests/python in my environment (python2 & python3, mkl, cuda, no mkldnn, no cudnn). There were three problems in the cython module.

setup.py is not compatible with cython 0.28. It is fixed.
The shared library files generated by cython have symbols defined in libmxnet.so, but the dynamic linker failed to find libmxnet.so unless LD_LIBRARY_PATH is set to the installation directory. This caused import error at runtime but the default behavior of mxnet is to try cython first and fallback to ctypes silently if an import error is encountered. So users cannot catch the error unless they set MXNET_ENFORCE_CYTHON env variable. This PR provides default search paths for libmxnet.so via rpath so that the linker can find it without LD_LIBRARY_PATH.
It looks like that the cython module for ndarray has been broken for months. It caused segfault due to the lack of a proper support for sparse arrays. This is also fixed.

asitstands · 2018-05-30T04:53:18Z

@szha @marcoabreu I think that it is necessary to add a cython build to CI. I'm not familiar with the CI system. Would you help?

asitstands · 2018-06-14T02:43:04Z

@piiswrong @szha @marcoabreu Would you review this PR?

marcoabreu · 2018-06-14T04:59:26Z

Sure. How can I help you?

asitstands · 2018-06-14T05:10:54Z

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

marcoabreu · 2018-06-14T05:13:05Z

Depends, I'm not familiar with with cython :) Does it require its own pipeline or is it just a compile-flag we can turn on?

Everything of interest to you would be in Jenkinsfile and ci/docker, right.

asitstands · 2018-06-14T05:28:02Z

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.

marcoabreu · 2018-06-14T14:18:54Z

Hmmm so far we don't use setup.py but just set the Python path. But I'd welcome to see that. Feel free to change that for some tests. What artifacts are being generated by running make in cython? We would have to take care of copying them over as well Deokjae Lee <[email protected]> schrieb am Mi., 13. Juni 2018, 22:28:

…

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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10951 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARxB67TrDKvLVuDn9aksojbVF5bAg0qTks5t8fR9gaJpZM4T_ovN> .

asitstands · 2019-04-15T04:30:21Z

Can we get this PR merged? I'm not sure what prevents the merging. Anyway MXNet does not use cython modules unless the env var MXNET_USE_CYTHON is set in runtime. So this PR does not change any behavior of MXNet unless the env var is set. Merging this PR is relatively safe and we could observe its effect after merging. Or we could completely remove the cython modules. They exist but are broken. Keeping the broken things would not be a good idea.

sl1pkn07 · 2019-04-27T12:37:02Z

broken again if use with master

larroy · 2019-05-01T23:23:08Z

Agree we should move this PR forward, or do something about it. Has been open for too long.

larroy · 2019-05-01T23:23:59Z

Do we need to rebase it? the patch shows unrelated diffs.

larroy · 2019-05-02T19:06:56Z

@asitstands could you rebase so we try to get this merged?

asitstands · 2019-05-03T06:42:25Z

I'm not sure why this PR shows unrelated commits with hundreds of changed files. My own repository shows only 13 files actually changed by this PR. I found that the target branch of this PR has been changed to apache:cython_fix from apache:master. Any relation with this? @szha

szha · 2019-05-03T18:05:13Z

@asitstands as this PR has been around for quite a while I changed it to a feature branch with the hope to merge it there so that others may carry on this work. Since you're working on this now, it's no longer necessary so I changed it back. It should be ready for review after another rebase.

asitstands · 2019-05-24T11:22:33Z

Now this PR has no confliction with the main branch and passes the tests. Please review whether it is OK to be merged.

* Fix broken build with cython 0.28 * Fix setup.py to be compatible with cython 0.28 * Fix broken cython ndarray module * Revised comments * Replace hard coded library path with one obtained by find_lib_path * Add documentation for MXNET_ENABLE_CYTHON and MXNET_ENFORCE_CYTHON * Add cython build to CI * Fix for cython CI * Adjust python environment for cython CI * Add make variables to set python executable * Fix typo * Fix nnvm include path * Does not use ccache for cython * Fix issues with the wildcards in the library list in Jenkinsfile * Fix issues with the wildcards in the library list in Jenkinsfile (continued) * Fix issues with the wildcards in the library list in Jenkinsfile (continued) * Intentionally introduce a bug to check that the tests actually runwith cython * Remove the intentionally introduced bug * Update installation doc * Retrigger CI * Run cython CI in ubuntu environment instead of CentOS environment * Commit a missed file * Fix a bug in check_cython * Refine environments for cython CI * Restore unrelated changes * Fix a problem occurring when the cython modules for python 2 and 3 are built successively * Trigger CI * Pin the cython version in the CI * Catch up apache#11320 * Remove optional arguments unused after apache#11320 * Trigger CI * Trigger CI * Trigger CI * Remove unnecessary stype argument from the NDArrayBase constructor * Revise confusing initialization of `_ndarray_cls` * Add cython build for python3 in CI * Fix misplaced cython build in CI * Adjust CI environments for cython * Fix invalid path for cython generated .so files in cmake build * Revert invalid fix * Revise docs * Revise check_cython * Temporaily use make instead of cmake for debugging * Temporal changes for debugging * Temporal changes for debugging * Temporaily use ctypes instead of cython modules for debugging * Temporaily disable ccache for debugging * Temporaily use make (DEV = 0) instead of cmake for debugging * Temporaily disable cudnn for debugging * Restore temporal changes * Temporarily disable coverage report * Adapt to Jenkinsfile_utils * Adapt to Jenkinsfile_utils (cont.) * Restore unrelated changes * Restore temporal changes * Resolving conflict * Test with the cmake build is removed * Add MXNET_ENABLE_CYTHON=0 to tensorrt test * Fix typo * Trigger CI * Trigger CI * Adapt to Jenkinsfile refactoring * Adapt to Jenkinsfile refactoring (cont.) * Trigger CI * Trigger CI * Stash missing cython modules * Trigger CI * CMake build of cython modules without unit tests * Fix typo * Trigger CI * Fix a mistake introduced in merging process * trigger test * Update Jenkinsfile_utils.groovy * Trigger CI * Trigger CI * Trigger CI * Trigger tests * Trigger tests * Trigger tests * Trigger tests * Trigger tests * Trigger tests * Trigger tests * Trigger tests

asitstands requested a review from szha as a code owner May 15, 2018 14:16

asitstands changed the title ~~Fix broken build with cython 0.28~~ Fix broken cython build May 26, 2018

asitstands force-pushed the fix_cython branch from 4b347d8 to ab555d8 Compare May 26, 2018 13:53

asitstands changed the title ~~Fix broken cython build~~ [MXNET-545] Fix broken cython build Jun 14, 2018

asitstands added 6 commits June 16, 2018 14:05

Fix broken build with cython 0.28

9d47eed

Fix setup.py to be compatible with cython 0.28

b3a9784

Fix broken cython ndarray module

feecd35

Revised comments

fcc41a0

Replace hard coded library path with one obtained by find_lib_path

771a9b6

Add documentation for MXNET_ENABLE_CYTHON and MXNET_ENFORCE_CYTHON

df1a291

asitstands force-pushed the fix_cython branch from 606856b to 0000950 Compare June 16, 2018 06:17

asitstands requested a review from marcoabreu as a code owner June 16, 2018 06:17

asitstands added 6 commits June 16, 2018 15:28

Add cython build to CI

0000950

Fix for cython CI

86d1c90

Adjust python environment for cython CI

f8a7b8c

Add make variables to set python executable

646ea91

Fix typo

64057b3

Fix nnvm include path

5aaa8ad

asitstands requested review from sergeykolychev and yzhliu as code owners April 13, 2019 05:28

Resolving conflicts with master

4be2da6

sergeykolychev approved these changes Apr 15, 2019

View reviewed changes

Merge branch 'master' into fix_cython

0dce3f9

szha changed the base branch from cython_fix to master May 3, 2019 18:03

asitstands added 13 commits May 6, 2019 18:43

Merge branch 'master' into fix_cython

ec37d87

Trigger tests

d5de9c4

Merge branch 'master' into fix_cython

11e30ca

Trigger tests

d8c435c

Merge branch 'master' into fix_cython

bc526f2

Merge branch 'master' into fix_cython

25a3dfd

Trigger tests

e300834

Trigger tests

8dbc12f

Trigger tests

10bc51e

Trigger tests

a6005c7

Trigger tests

db80ee7

Merge branch 'master' into fix_cython

a5ad3c0

Trigger tests

1443088

szha merged commit d0ff3cd into apache:master May 24, 2019

hubutui mentioned this pull request Jun 18, 2019

question about BUILD_CYTHON_MODULES when building with cmake #15263

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-545] Fix broken cython build #10951

[MXNET-545] Fix broken cython build #10951

asitstands commented May 15, 2018 •

edited

Loading

szha commented May 21, 2018

asitstands commented May 22, 2018

szha commented May 22, 2018 •

edited

Loading

asitstands commented May 22, 2018 •

edited

Loading

asitstands commented May 26, 2018 •

edited

Loading

asitstands commented May 30, 2018

asitstands commented Jun 14, 2018

marcoabreu commented Jun 14, 2018

asitstands commented Jun 14, 2018 •

edited

Loading

marcoabreu commented Jun 14, 2018

asitstands commented Jun 14, 2018 •

edited

Loading

marcoabreu commented Jun 14, 2018 via email

asitstands commented Apr 15, 2019 •

edited

Loading

sl1pkn07 commented Apr 27, 2019

larroy commented May 1, 2019

larroy commented May 1, 2019 •

edited

Loading

larroy commented May 2, 2019

asitstands commented May 3, 2019

szha commented May 3, 2019

asitstands commented May 24, 2019

[MXNET-545] Fix broken cython build #10951

[MXNET-545] Fix broken cython build #10951

Conversation

asitstands commented May 15, 2018 • edited Loading

Description

Checklist

Essentials

szha commented May 21, 2018

asitstands commented May 22, 2018

szha commented May 22, 2018 • edited Loading

asitstands commented May 22, 2018 • edited Loading

asitstands commented May 26, 2018 • edited Loading

asitstands commented May 30, 2018

asitstands commented Jun 14, 2018

marcoabreu commented Jun 14, 2018

asitstands commented Jun 14, 2018 • edited Loading

marcoabreu commented Jun 14, 2018

asitstands commented Jun 14, 2018 • edited Loading

marcoabreu commented Jun 14, 2018 via email

asitstands commented Apr 15, 2019 • edited Loading

sl1pkn07 commented Apr 27, 2019

larroy commented May 1, 2019

larroy commented May 1, 2019 • edited Loading

larroy commented May 2, 2019

asitstands commented May 3, 2019

szha commented May 3, 2019

asitstands commented May 24, 2019

asitstands commented May 15, 2018 •

edited

Loading

szha commented May 22, 2018 •

edited

Loading

asitstands commented May 22, 2018 •

edited

Loading

asitstands commented May 26, 2018 •

edited

Loading

asitstands commented Jun 14, 2018 •

edited

Loading

asitstands commented Jun 14, 2018 •

edited

Loading

asitstands commented Apr 15, 2019 •

edited

Loading

larroy commented May 1, 2019 •

edited

Loading