Dynamic Library Loading Support #15489

mseth10 · 2019-07-08T23:30:39Z

Description

This PR is part of a larger effort of adding new functionality to MXNet that was compiled separately from MXNet. See here and here for more info.

This PR is the first step that enables libraries to be dynamically loaded into MXNet at runtime .

Features:

Dynamic library loading functions for both Linux and Windows to load libraries and access its functions
MXLoadLib that loads and initializes a library
A new Python API: mx.library.load that calls the new C-API
Enhancements to both the Makefile and CMake flows to link with the dynamic loader library (dl)

This PR includes the following:

An example, showing how to write a sample library in C++ (mylib.cc) with its own Makefile, a unittest to verify correctness outside of MXNet (libtest.cc), and a test script showing how to load the library in MXNet (test.py).
A unit test that loads a precompiled library into MXNet and initializes it. It runs as part of the CI for both Linux and Windows.

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

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

karan6181 · 2019-07-15T21:30:12Z

@mxnet-label-bot add [backend, C API, pr-work-in-progress]

samskalicky · 2019-08-03T17:42:23Z

@leleamol Can you please review again to see if the implementation of the dlclose matches what you were thinking?

@anirudh2290 Thanks for the help with the engine destructor info, can you take a look at the changes to the engines:
https://github.com/apache/incubator-mxnet/blob/134cc37d6b306514fde1a405d3b87cc946a56bd1/src/engine/naive_engine.cc#L88-L91
https://github.com/apache/incubator-mxnet/blob/134cc37d6b306514fde1a405d3b87cc946a56bd1/src/engine/threaded_engine.h#L335-L338

concern no longer applies

leleamol

Looks good to me.

leleamol · 2019-08-05T17:50:03Z

src/common/library.cc

+    LocalFree(err_msg);
+  }
+#else
+  *func = dlsym(handle, name);


Ok.. Sounds good.

rondogency

LGTM

This reverts commit 3112893.

marcoabreu · 2019-08-05T19:31:20Z

tests/python/unittest/test_library_loading.py

+    elif (os.name=='nt'):
+        lib = 'mylib.dll'
+
+    fname = mx.test_utils.download('https://mxnet-demo-models.s3.amazonaws.com/lib_binary/'+lib)


@mseth10

Unfortunately, this test introduces a security vulnerability. Please replace it as soon as possible as this poses a security threat to our system as well as our users since this allows arbitrary code execution. I'd appreciate it if you would provide a replacement within the next 24h. #15755 is the corresponding revert PR.

I suggested before to build this stub during the build process instead of loading from S3. See previous comment above: https://github.com/apache/incubator-mxnet/pull/15489/files#r308932209

samskalicky · 2019-08-05T20:47:09Z

Thanks @marcoabreu for the catch. As we discussed offline, the problem is not with the feature in this PR but with how test_library_loading is pulling from the S3 bucket in the CI.

We'll work to build the library as part of the build so that it can be used internally in testing the CI without security risks. We'll move the test pulling from the S3 bucket to the examples directory so that it doesnt run in the CI.

We may need some help @marcoabreu to get this working in the CI, since we'll need to build this library and then find the path where it lives during the test so we can pass the path to the load API.

In the meantime, we'll make these changes and then resubmit this PR.

marcoabreu · 2019-08-05T21:04:29Z

We'll move the test pulling from the S3 bucket to the examples directory so that it doesnt run in the CI.

Could you elaborate?

samskalicky · 2019-08-05T21:20:30Z

@marcoabreu we added an example directory at: example/lib_api. Currently the test.py includes a test that uses the locally built library (built using the included Makefile). But this only provides an example for how to build a library.

We'll create another example python script that pulls from the S3 bucket and loads that one. But it will just be an example, not a unittest that will run in the CI.

Do you have other ideas/suggestions on how we can improve testing or provide more examples?

marcoabreu · 2019-08-05T22:54:54Z

No pulling of any prebuilt binaries due to the same reasons, please. I understand you trying to make it more convenient, but I think that we're stepping over a line in that case. Give instructions how to compile the example and the user should be fine.

Just to confirm: we will have a unit test running in ci at the end of the day, correct?

samskalicky · 2019-08-05T23:10:40Z

@marcoabreu I think we're in agreement here. We will be distributing some pre-built libraries in an S3 bucket as examples. We will provide documented examples for how to pull and load these binaries. But these will not be tests that will be automatically run. They will be examples.

Yes, we will have the unit test running in the CI that uses a library that is built in the CI.

* Revert "Dynamic Library Loading Support (#15489)" This reverts commit 3112893. * Retrigger CI * CI * retrigger CI * retrigger CI * retrigger CI

wkcn · 2019-08-07T12:25:00Z

I have a suggestion. We can check the sha1_hash code of pre-built binary files in the S3 bucket before executing them.

marcoabreu · 2019-08-07T12:30:42Z

While that eliviates the problem from a ci perspective (if the bucket would be owned by the CI owners), it's still a red flag for our users.

I'm afraid that companies prefer to stay away from the execution of an arbitrary native library that is downloaded from the internet - no matter whether it is backed by a hash or not. It's inevitable in some cases, but we want to reduce the possiblity of compliance scanners flagging us and thus making the usage of the MXNet more complicated than it has to be.

I think the only acceptable way is if the library is getting produced as part of the build process that is entirely retrieved from source. These are also the tenants Apache employs - Apache project must not make any binary releases in any form. Thus let's best stick to that as well :)

* Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu

* Dynamic Library Loading Support (#15489) * Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu * build library and stash it in CI * modifying unittest to use CI built binary * Retrigger CI * adding dlclose to initialize destructor * adding sample_lib target to all in Makefile

* Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu

* Revert "Dynamic Library Loading Support (apache#15489)" This reverts commit 3112893. * Retrigger CI * CI * retrigger CI * retrigger CI * retrigger CI

* Dynamic Library Loading Support (apache#15489) * Accelerator APIs header file * adding example to test accelerator loading * adding c_api function header * modifying header file path * creating templates for call to lib fns * modifying target to mxnet_static for libdl * rebaseing with master * returning nullptr handle if library not loaded * refactoring code to load libraries dynamically * addressing review comments * using static cast * pylint fix * moving library.h file to src/common/ * adding dynamic loading support for windows * updating header guard * fixing headers * fixing windows casting error * declaring library functions for windows * adding library testing module in examples * adding unit test to test library loading * correcting file names * updating error messages * getting error message from DL library * adding unit test to gpu suite * correcting windows pointer * requiring absolute path to library * changing file description * addressing review comments - adding more docs, windows error msg * addressing PR comments * checking machine type for unit test * “re-trigger” * added map to store loaded libraries * added dlclose calls in naive & threaded engines * removed library map declaration in cc file * added windows free * fixed formatting * added cast to HMODULE for void* for windows * retrigger CI for flaky unix_cpu * build library and stash it in CI * modifying unittest to use CI built binary * Retrigger CI * adding dlclose to initialize destructor * adding sample_lib target to all in Makefile

mseth10 requested review from anirudh2290 and eric-haibin-lin as code owners July 8, 2019 23:30

mseth10 force-pushed the acc_api branch 8 times, most recently from cc55e0d to 94fda6e Compare July 10, 2019 00:50

mseth10 requested a review from szha as a code owner July 11, 2019 01:02

mseth10 force-pushed the acc_api branch 14 times, most recently from 671df6f to ddd0e59 Compare July 12, 2019 21:59

mseth10 requested a review from rahul003 as a code owner July 12, 2019 21:59

mseth10 force-pushed the acc_api branch 3 times, most recently from 448e950 to c490c23 Compare July 15, 2019 09:18

samskalicky added 3 commits August 2, 2019 06:46

added dlclose calls in naive & threaded engines

0d05d1c

removed library map declaration in cc file

3879f61

added windows free

289c3a5

samskalicky force-pushed the acc_api branch from 672b2b9 to b62aaf3 Compare August 2, 2019 18:04

samskalicky added 3 commits August 2, 2019 18:29

fixed formatting

a3b545d

added cast to HMODULE for void* for windows

e512d64

retrigger CI for flaky unix_cpu

134cc37

leleamol approved these changes Aug 5, 2019

View reviewed changes

src/common/library.cc

LocalFree(err_msg);

}

#else

*func = dlsym(handle, name);

Copy link

Contributor

leleamol Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.. Sounds good.

rondogency approved these changes Aug 5, 2019

View reviewed changes

eric-haibin-lin merged commit 3112893 into apache:master Aug 5, 2019

marcoabreu added a commit that referenced this pull request Aug 5, 2019

Revert "Dynamic Library Loading Support (#15489)"

1e97b86

This reverts commit 3112893.

marcoabreu mentioned this pull request Aug 5, 2019

Revert "Dynamic Library Loading Support" #15755

Merged

marcoabreu reviewed Aug 5, 2019

View reviewed changes

mseth10 mentioned this pull request Aug 6, 2019

Fix PR #15489 (Dynamic Library Loading Support) #15760

Merged

7 tasks

wkcn pushed a commit that referenced this pull request Aug 7, 2019

Revert "Dynamic Library Loading Support" (#15755)

7186123

* Revert "Dynamic Library Loading Support (#15489)" This reverts commit 3112893. * Retrigger CI * CI * retrigger CI * retrigger CI * retrigger CI

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Library Loading Support #15489

Dynamic Library Loading Support #15489

mseth10 commented Jul 8, 2019 •

edited

Loading

karan6181 commented Jul 15, 2019

samskalicky commented Aug 3, 2019 •

edited

Loading

leleamol left a comment

leleamol Aug 5, 2019

rondogency left a comment

marcoabreu Aug 5, 2019 •

edited

Loading

larroy Aug 5, 2019 •

edited

Loading

marcoabreu Aug 5, 2019

samskalicky commented Aug 5, 2019 •

edited

Loading

marcoabreu commented Aug 5, 2019

samskalicky commented Aug 5, 2019 •

edited

Loading

marcoabreu commented Aug 5, 2019

samskalicky commented Aug 5, 2019

wkcn commented Aug 7, 2019

marcoabreu commented Aug 7, 2019 •

edited

Loading

Dynamic Library Loading Support #15489

Dynamic Library Loading Support #15489

Conversation

mseth10 commented Jul 8, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Comments

karan6181 commented Jul 15, 2019

samskalicky commented Aug 3, 2019 • edited Loading

leleamol left a comment

Choose a reason for hiding this comment

leleamol Aug 5, 2019

Choose a reason for hiding this comment

rondogency left a comment

Choose a reason for hiding this comment

marcoabreu Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

larroy Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

marcoabreu Aug 5, 2019

Choose a reason for hiding this comment

samskalicky commented Aug 5, 2019 • edited Loading

marcoabreu commented Aug 5, 2019

samskalicky commented Aug 5, 2019 • edited Loading

marcoabreu commented Aug 5, 2019

samskalicky commented Aug 5, 2019

wkcn commented Aug 7, 2019

marcoabreu commented Aug 7, 2019 • edited Loading

mseth10 commented Jul 8, 2019 •

edited

Loading

samskalicky commented Aug 3, 2019 •

edited

Loading

marcoabreu Aug 5, 2019 •

edited

Loading

larroy Aug 5, 2019 •

edited

Loading

samskalicky commented Aug 5, 2019 •

edited

Loading

samskalicky commented Aug 5, 2019 •

edited

Loading

marcoabreu commented Aug 7, 2019 •

edited

Loading