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

[MXNET-1110] Add header files required by horovod #13062

Merged
merged 17 commits into from
Dec 5, 2018

Conversation

apeforest
Copy link
Contributor

@apeforest apeforest commented Oct 31, 2018

Description

When integrating mxnet with horovod, the callback functions need to use certain MXNet APIs which are not currently explosed in include directory. This PR adds the required header files to the include directory as soft links. The header files are already exported in the pip package and verified on Horovod side.

@apeforest
Copy link
Contributor Author

@szha Your review and advice are very much appreciated.

@rahul003
Copy link
Member

rahul003 commented Oct 31, 2018

Do we need the headers for the submodules too? That would also become really hard to maintain.

@ankkhedia
Copy link
Contributor

@apeforest Thanks for your contribution!
@mxnet-label-bot [pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Oct 31, 2018
@apeforest
Copy link
Contributor Author

@rahul003 Yes, because mxnet/engine.h --> mxnet/base.h --> All these submodules.

@kalyc
Copy link
Contributor

kalyc commented Nov 12, 2018

@apeforest thanks for your contribution. Could you please update your branch and re-trigger CI?

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

@harshp8l
Copy link
Contributor

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

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Nov 12, 2018
@apeforest apeforest requested a review from szha as a code owner November 20, 2018 00:56
@apeforest apeforest changed the title [WIP] Add header files required by horovod Add header files required by horovod Nov 20, 2018
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. How are you verifying this change?

python/mxnet/libinfo.py Outdated Show resolved Hide resolved
@vandanavk
Copy link
Contributor

@yuxihu @Vikas89 for review

@apeforest apeforest changed the title Add header files required by horovod [MXNET-1110] Add header files required by horovod Dec 4, 2018
Copy link
Member

@anirudh2290 anirudh2290 left a comment

Choose a reason for hiding this comment

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

can we address the other open questions in the review.

ci/build_windows.py Outdated Show resolved Hide resolved
@apeforest
Copy link
Contributor Author

@rahul003 Yes, all the added header files are required by Horovod.

@apeforest
Copy link
Contributor Author

@szha Yes, we have verified that Horovod works well with MXNet with these included header files in pip package.

Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

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

LGTM.

@apeforest
Copy link
Contributor Author

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Dec 5, 2018
@apeforest
Copy link
Contributor Author

@anirudh2290 Can this PR be merged now?

@anirudh2290 anirudh2290 merged commit 2f55488 into apache:master Dec 5, 2018
zhaoyao73 pushed a commit to zhaoyao73/incubator-mxnet that referenced this pull request Dec 13, 2018
* Add header files required by horovod

* Add symbolic link and cherry picked required header

* add python API to return include path

* update link

* fix windows CI

* fix windows build

* fix dlpack link

* merge with master

* exclude 3rd party header files from license check

* exclude license check

* exclude include directory

* remove commented lines
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 9, 2019
* Add header files required by horovod

* Add symbolic link and cherry picked required header

* add python API to return include path

* update link

* fix windows CI

* fix windows build

* fix dlpack link

* merge with master

* exclude 3rd party header files from license check

* exclude license check

* exclude include directory

* remove commented lines
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 14, 2019
* Add header files required by horovod

* Add symbolic link and cherry picked required header

* add python API to return include path

* update link

* fix windows CI

* fix windows build

* fix dlpack link

* merge with master

* exclude 3rd party header files from license check

* exclude license check

* exclude include directory

* remove commented lines
apeforest added a commit to apeforest/incubator-mxnet that referenced this pull request Feb 5, 2019
* Add header files required by horovod

* Add symbolic link and cherry picked required header

* add python API to return include path

* update link

* fix windows CI

* fix windows build

* fix dlpack link

* merge with master

* exclude 3rd party header files from license check

* exclude license check

* exclude include directory

* remove commented lines
apeforest added a commit to apeforest/incubator-mxnet that referenced this pull request Apr 25, 2019
* Add header files required by horovod

* Add symbolic link and cherry picked required header

* add python API to return include path

* update link

* fix windows CI

* fix windows build

* fix dlpack link

* merge with master

* exclude 3rd party header files from license check

* exclude license check

* exclude include directory

* remove commented lines
@apeforest apeforest deleted the feature/horovod-integration branch August 23, 2019 17:09
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.