Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

External Operators 2 #19431

Merged
merged 7 commits into from
Nov 14, 2020
Merged

External Operators 2 #19431

merged 7 commits into from
Nov 14, 2020

Conversation

samskalicky
Copy link
Contributor

@samskalicky samskalicky commented Oct 27, 2020

Description

This is a follow-on PR after the initial investigation in #18904 to support loading libraries containing MXNet backend components (particularly operators). Since #19241, the version script was removed and now Linux builds of libmxnet.so expose all MXNet symbols. This opens the door for dynamically loading libraries that depend on libmxnet.so.

This PR adds a build option BUILD_EXTENSION to the CMakeLists.txt file to give a path to the extension directory to build. This enables the existing MXNet build infrastructure to provide all includes directories automatically which simplifies building extensions.

An example is also added in examples/extensions/lib_external_ops that builds an operator that is equivalent to a built-in/backend operator into a shared object and uses the existing mx.library.load API to load that library at runtime.

Caveots

Windows is not supported since by default all symbols are hidden in mxnet.dll so dynamically loading libraries will not work. Similarly with Mac, the version script does not expose the necessary symbols at this time. These other platforms can be considered in the future based on user request & community motivation.

@mxnet-bot
Copy link

Hey @samskalicky , 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-cpu, clang, unix-cpu, sanity, edge, website, centos-cpu, unix-gpu, centos-gpu, miscellaneous, windows-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.

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 27, 2020
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 27, 2020
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

@samskalicky Thank you!
I have two suggestions : )

CMakeLists.txt Outdated Show resolved Hide resolved
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Oct 31, 2020
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for the contribution!

@lanking520 lanking520 added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 31, 2020
Copy link
Contributor

@mseth10 mseth10 left a comment

Choose a reason for hiding this comment

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

LGTM

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Nov 6, 2020
@lanking520 lanking520 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 6, 2020
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Nov 6, 2020
@lanking520 lanking520 added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 6, 2020
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-awaiting-review PR is waiting for code review pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 6, 2020
@samskalicky
Copy link
Contributor Author

Thanks @mseth10 for adding the tests to the CI!

@leezu anything else we need here before taking the next step?

@samskalicky samskalicky added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Nov 7, 2020
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Nov 12, 2020
@lanking520 lanking520 added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 13, 2020
@samskalicky samskalicky merged commit 6bc0647 into apache:master Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants