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

Cmake blas #14871

Closed
wants to merge 3 commits into from
Closed

Cmake blas #14871

wants to merge 3 commits into from

Conversation

edisongustavo
Copy link
Contributor

This is a re-creation of #14028. I've accidentally deleted my fork of incubator-mxnet.

Description

Eases the pain of linking with OpenBLAS using cmake.

This PR adds support to use the provided OpenBLASConfig.cmake.

Justification for the change

The standard way of linking with dependencies in CMake is via the find_package() mechanism (docs). This mechanism provides 2 ways of finding the dependencies:

  • Config mode (uses a file named OpenBLASConfig.cmake (or openblas-config.cmake), which are provided by the dependency itself)
  • Module mode (uses a file named FindOpenBLAS.cmake, which is provided by either cmake or the project compiling the dependency, mxnet in this case)

The preferred way to find a dependency is if they provide their "Config" file, since the authors of the dependency are the ones who know best about the structure of their code and how they should be linked.

When I tried to compile MXNet on Windows it didn't work well. My setup was:

It failed because this binary package is compiled with a Visual Studio version prior to 2015. So I would get the error unresolved external symbol __imp____iob_func. The reasons are explained here: https://stackoverflow.com/questions/30412951/unresolved-external-symbol-imp-fprintf-and-imp-iob-func-sdl2

Then I tried to use the conda-forge package. This almost worked.

On further inspection of the FindOpenBLAS.cmake and the conda-forge package, I noticed that MXNet didn't use the provided "cmake Config mode" files (OpenBLASConfig.cmake). It tried to find the library all by itself, which is an "anti-pattern" when using cmake. So I thought that this could be an opportunity to be able to contribute to the project by improving this module.

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

  • Add , tests, (and when applicable, API doc)

I have tested this by compiling it on Windows and Linux.

To provide OpenBLAS I have tried:

On Linux:

  • compiled OpenBLAS from source with cmake and installed it
  • compiled OpenBLAS from source with make and installed it

On Windows:

Comments

@edisongustavo edisongustavo requested a review from szha as a code owner May 3, 2019 09:37
@edisongustavo edisongustavo mentioned this pull request May 3, 2019
6 tasks
@anirudhacharya
Copy link
Member

@mxnet-label-bot add [pr-awaiting-review]

cc @KellenSunderland @larroy for review

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label May 3, 2019
@yinghu5
Copy link
Contributor

yinghu5 commented May 6, 2019

in the PR #14877, we only change the MKLDNN and MKL part, and keep the OpenBLAS untouched.

About #14028, agree the latest solution, I will update the install guide of windows

If you don't have the Intel Math Kernel Library (MKL) installed, download and install OpenBLAS. Note that you should also download ```mingw64.dll.zip`` along with openBLAS and add them to PATH.
make sure the OpenBLAS to point to use version 0.3.6 instead. thanks

@aaronmarkham
Copy link
Contributor

@edisongustavo I think the suggested docs changes from your earlier PR will be fine. Please add those changes and that will also trigger CI. However, it looks like your last commit is pretty old, so you probably should rebase first.

@edisongustavo
Copy link
Contributor Author

@aaronmarkham Ok, I will do that!

@pinaraws
Copy link

@mxnet-label-bot add[Build, pr-awaiting-response]

@marcoabreu marcoabreu added Build pr-awaiting-response PR is reviewed and waiting for contributor to respond labels May 20, 2019
@pinaraws
Copy link

@mxnet-label-bot remove[pr-awaiting-review]

@marcoabreu marcoabreu removed the pr-awaiting-review PR is waiting for code review label May 20, 2019
${OpenBLAS_LIB_SEARCH_PATHS}

"$ENV{OpenBLAS_HOME}"
"${OpenBLAS_HOME}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need Openblas_home/include? Where is this list coming from?

@piyushghai
Copy link
Contributor

@edisongustavo Can you address the review comments by @larroy ?

Also can you rebase with the latest master branch ?

@larroy
Copy link
Contributor

larroy commented Jun 20, 2019

@edisongustavo could you please look at the CI failures and the linked github issue? Thank you!

@karan6181
Copy link
Contributor

@edisongustavo Could you please re-trigger the CI?

@piyushghai
Copy link
Contributor

@edisongustavo Gentle ping ...

@edisongustavo
Copy link
Contributor Author

Hello everyone, I don't think I will be able to work on this anymore.

Given that Conan support has been merged in #13400, then I think this PR is obsolete.

Re-open if you think I'm wrong, but this is just dragging forever.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Build pr-awaiting-response PR is reviewed and waiting for contributor to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants