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

[BUGFIX] Fix for MKL symbol name clashing issue (#18855) #19140

Closed
wants to merge 1 commit into from

Conversation

akarbown
Copy link
Contributor

@akarbown akarbown commented Sep 14, 2020

Description

This change enables symbol exclusion of statically
linked MKL libraries to avoid the name clashing issue #18855.

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • Code is well-documented

This change enables symbol exclusion of statically
linked MKL libraries to avoid the name clashing issue.
@mxnet-bot
Copy link

Hey @akarbown , 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: [centos-gpu, sanity, windows-cpu, unix-gpu, unix-cpu, website, centos-cpu, edge, windows-gpu, miscellaneous, clang]


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-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review labels Sep 14, 2020
Copy link
Contributor

@leezu leezu left a comment

Choose a reason for hiding this comment

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

This option applies globally and affects all libraries statically linked into libmxnet.so. Thus the behavior should not depend on if MKL is disabled or enabled.

Also note that the option is not supported on Mac, so we may need to use a explicit symbol whitelist for consistency. #19057 has a similar change affecting only the distribution (think pypi) scripts.

@akarbown
Copy link
Contributor Author

This option applies globally and affects all libraries statically linked into libmxnet.so. Thus the behavior should not depend on if MKL is disabled or enabled.

Also note that the option is not supported on Mac, so we may need to use a explicit symbol whitelist for consistency. #19057 has a similar change affecting only the distribution (think pypi) scripts.

Hi @leezu
Thanks for the review!
I see that you've done all the changes needed to avoid symbol clashing issue but for mxnet distribution version. I'd like to ask you whether it should be done for all the build types not only for distribution? So that all the developers would have it by default.

@leezu
Copy link
Contributor

leezu commented Sep 23, 2020

@akarbown yes, it's fine to enable by default. The main challenge is that you need to ensure not to break several special cases, such as compilation on different operating systems and different compilers.

@leezu
Copy link
Contributor

leezu commented Sep 25, 2020

For example, the current approach won't work on macOS.

To support macOS, you could switch the following lines to always apply (not only in the distribution build):

https://github.com/apache/incubator-mxnet/blob/b225fa5d3c9b6d9ba10533fcbf20a20c6aa96aff/CMakeLists.txt#L673-L677

@akarbown akarbown closed this Oct 23, 2020
@akarbown akarbown deleted the stat_ilp64 branch November 16, 2020 10:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
MKLDNN pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants