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

[MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs #15177

Merged
anirudh2290 merged 4 commits into from
Jun 14, 2019
Merged

[MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs #15177

anirudh2290 merged 4 commits into from
Jun 14, 2019

Conversation

@wkcn
Copy link
Member

@wkcn wkcn commented Jun 7, 2019 •
edited
Loading

Description

Hi, there.
In the PR #14615, @yuxihu added Add MXEnginePushAsync and MXEnginePushSync C APIs.

MXEnginePushAsync and MXEnginePushSync needs VarHandles of NDArrays.
However, MXNet doesn't expose the API to get the VarHandle of a NDArray.

After the discussion with @szha, We considered that it's more elegant to add MXEnginePushAsyncND and MXEnginePushSyncND C APIs, whose arguments are NDArrayHandles, rather than VarHandles.

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

@wkcn wkcn changed the title Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs [MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs Jun 7, 2019
@wkcn wkcn added the C++ Related to C++ label Jun 7, 2019
@wkcn wkcn added the pr-awaiting-review PR is waiting for code review label Jun 7, 2019
@leleamol
Copy link
Contributor

leleamol commented Jun 7, 2019 •
edited
Loading

The proposed APIs are not part of cpp-package C++ API. Hence removing the "C++" label.
@mxnet-label-bot remove [C++]

@marcoabreu marcoabreu removed the C++ Related to C++ label Jun 7, 2019
anirudh2290
anirudh2290 reviewed Jun 7, 2019
View reviewed changes
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.

@wkcn What is the additional use case for this apart from horovod ? Is this going to be changed in horovod ?

@wkcn
Copy link
Member Author

wkcn commented Jun 7, 2019

@anirudh2290 I will write a use case to test the two APIs : ), and it does not change Horovod. It is useful for Horovod to avoid including different versions of ndarray header file. In addition, it will be better for TVM and other third-party libraries.

I write a custom operator library MobulaOP (https://github.com/wkcn/mobulaop), and I found that there is not any way to use the asynchronous execution of MXNet. Although we can use TVMBridge API (https://github.com/apache/incubator-mxnet/blob/master/src/nnvm/tvm_bridge.cc#L174) to create an asynchronous function, there is an ABI compatibility problem (We discussed it on TVM forum https://discuss.tvm.ai/t/the-abi-compatibility-of-packedfunc/1601), because TVMBridge uses the C++ class PackedFunc with std::function object.

@anirudh2290
Copy link
Member

anirudh2290 commented Jun 7, 2019

@wkcn thank you for providing the details!

@wkcn wkcn changed the title [MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs [WIP][MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs Jun 8, 2019
@wkcn wkcn added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Jun 8, 2019
@wkcn
Copy link
Member Author

wkcn commented Jun 8, 2019 •
edited
Loading

Hi, @anirudh2290 . I have written a simple example to show hot to use these two APIs.
https://github.com/wkcn/test_mx_sync

Test Procedure:

  1. Build MXNet with this PR
  2. git clone https://github.com/wkcn/test_mx_sync && cd test_mx_sync
  3. make
  4. python test_sync.py

@wkcn wkcn added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Jun 8, 2019
@wkcn wkcn changed the title [WIP][MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs [MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs Jun 8, 2019
@anirudh2290 anirudh2290 merged commit 3b663ef into apache:master Jun 14, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@wkcn
[MXNET-1415]Add MXEnginePushAsyncND and MXEnginePushSyncND C APIs (ap…
a9a0325 
…ache#15177)

* add MXEnginePushAsyncND and MXEnginePushSyncND

* fix test build

* return exception value

* retrigger CI
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

1 more reviewer

@anirudh2290 anirudh2290 anirudh2290 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wkcn @leleamol @anirudh2290 @marcoabreu