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

mseth10 · 2019-08-06T01:15:59Z

Description

This PR builds on top of #15489, which was reverted by #15755.

It includes the following additional changes:

Changes to Makefile and CMake flows to build sample library as part of MXNet build
Modifications to CI to stash/un-stash the library binary from build stage to test stage
Loads the binary as part of unit test

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

tests/python/unittest/test_library_loading.py

* 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

larroy · 2019-08-07T20:44:43Z

example/lib_api/Makefile

+# under the License.
+
+all:
+	g++ -shared -fPIC mylib.cc -o mylib.so -I ../../include/mxnet


If CI doesn't pass, I suggest to use $(CC) and $(CXX) and parameterize the linker so it's coming from environment as usual instead of hardcoded, if not it's not worth another CI round.

@larroy this isnt building in the CI. this is just an example. See changes in the other Makefile or CMakeLists.txt for changes to build the library in the CI

I understand, but still this is a PITA if a Makefile doesn't respect the compiler from the environment. That's why I said is not worth another CI round. If you make other changes I would change it. Or is there a reason not to?

How does that work for windows? will it automatically have CXX be "cl" ?

larroy

Two questions, and one issue regarding close. Rest looks good.

larroy · 2019-08-07T20:48:22Z

src/common/library.h

+ * \return func a function pointer
+ */
+template<typename T>
+T get_func(void *lib, char *func_name) {


We don't want to get funky inside a mxnet namespace?

src/engine/threaded_engine.h

Makefile

larroy

LGTM I will move loaded_libs to LibraryInitializer singleton, since I have a PR open I can do it there after this is merged.

larroy · 2019-08-08T00:42:00Z

src/common/library.h

+#include "dmlc/io.h"
+
+// map of libraries loaded
+static std::map<std::string, void*> loaded_libs;


This should not be static in a header. I will change it in my PR. This creates a symbol in every compilation unit.

* 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

Makefile

* 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 a review from szha as a code owner August 6, 2019 01:15

mseth10 commented Aug 6, 2019

View reviewed changes

tests/python/unittest/test_library_loading.py Outdated Show resolved Hide resolved

mseth10 changed the title ~~build sample library in CI~~ [WIP] build sample library in CI Aug 6, 2019

mseth10 force-pushed the ext_lib branch 4 times, most recently from 3192333 to aafed25 Compare August 6, 2019 21:52

mseth10 and others added 3 commits August 7, 2019 17:58

build library and stash it in CI

42fda42

modifying unittest to use CI built binary

bb12267

mseth10 force-pushed the ext_lib branch from 0f804cb to bb12267 Compare August 7, 2019 18:21

mseth10 requested review from anirudh2290 and eric-haibin-lin as code owners August 7, 2019 18:21

mseth10 changed the title ~~[WIP] build sample library in CI~~ Fix PR #15489 (Dynamic Library Loading Support) Aug 7, 2019

Retrigger CI

cb40a02

larroy reviewed Aug 7, 2019

View reviewed changes

larroy suggested changes Aug 7, 2019

View reviewed changes

adding dlclose to initialize destructor

8feb07d

zachgk reviewed Aug 7, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

adding sample_lib target to all in Makefile

ca608ff

zachgk approved these changes Aug 8, 2019

View reviewed changes

larroy approved these changes Aug 8, 2019

View reviewed changes

zachgk merged commit a2b11ae into apache:master Aug 8, 2019

larroy reviewed Aug 8, 2019

View reviewed changes

TaoLv reviewed Sep 24, 2019

View reviewed changes

Makefile Show resolved Hide resolved

samskalicky mentioned this pull request Sep 25, 2019

dynamic custom operator support #15921

Merged

4 tasks

samskalicky mentioned this pull request Dec 13, 2019

Dynamic subgraph property #17034

Merged

4 tasks

wkcn mentioned this pull request May 9, 2020

[MXNet Extensions] Include lib_api.h in the pre-built pip package #18267

Open

mseth10 deleted the ext_lib branch June 1, 2020 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mseth10 commented Aug 6, 2019 •

edited

Loading

larroy Aug 7, 2019 •

edited

Loading

samskalicky Aug 7, 2019 •

edited

Loading

larroy Aug 7, 2019

samskalicky Aug 7, 2019

larroy left a comment

larroy Aug 7, 2019

larroy left a comment

larroy Aug 8, 2019

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

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

Conversation

mseth10 commented Aug 6, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Comments

larroy Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

samskalicky Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

larroy Aug 7, 2019

Choose a reason for hiding this comment

samskalicky Aug 7, 2019

Choose a reason for hiding this comment

larroy left a comment

Choose a reason for hiding this comment

larroy Aug 7, 2019

Choose a reason for hiding this comment

larroy left a comment

Choose a reason for hiding this comment

larroy Aug 8, 2019

Choose a reason for hiding this comment

mseth10 commented Aug 6, 2019 •

edited

Loading

larroy Aug 7, 2019 •

edited

Loading

samskalicky Aug 7, 2019 •

edited

Loading