Fix the bug of `MXEnginePushAsyncND` and `MXEnginePushSyncND` #15751

wkcn · 2019-08-05T04:24:23Z

Description

Sorry that I wrote a bug in the two APIs MXEnginePushAsyncND and MXEnginePushSyncND, whose argument should be an array pointer of NDArrayHandle.

Issue: #15774
master branch: #15751
v1.5.x branch: #15775

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

Modify the type of arguments to 'NDArry**'
Update the unittest

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

wkcn · 2019-08-05T12:29:34Z

src/c_api/c_api.cc

+                        EngineFnPropertyHandle prop_handle, int priority,
+                        const char* opr_name, bool wait) {
+  API_BEGIN();
+  NDArray** const_nds = reinterpret_cast<NDArray**>(const_nds_handle);


reinterpret_cast is necessary for the cast from void** to NDArray**.

sxjscience · 2019-08-05T18:05:30Z

Why did the code pass the previous tests?

wkcn · 2019-08-05T21:57:35Z

@sxjscience The previous test is also wrong. In the previous test, I passed the memory address of a NDArray into these two APIs. However, the correct method is to pass the memory address of an array of NDarray pointers.

wkcn · 2019-08-06T16:20:41Z

@sxjscience @anirudh2290 @eric-haibin-lin
Hi, could you please help take a review? Thank you!

KellenSunderland · 2019-08-06T21:42:07Z

Should we backport this to the v1.5.x branch as well?

wkcn · 2019-08-06T21:54:26Z

@KellenSunderland Yes.

KellenSunderland · 2019-08-06T22:19:08Z

tests/cpp/engine/threaded_engine_test.cc

@@ -258,47 +258,50 @@ TEST(Engine, PushFunc) {
 TEST(Engine, PushFuncND) {
  auto ctx = mxnet::Context{};
  mxnet::NDArray nd(ctx);
+  std::vector<mxnet::NDArray*> nds;
+  nds.push_back(&nd);
+  void** pnds = reinterpret_cast<void**>(nds.data());


We might want some tests for cases with more than 1 NDArray present in the argument call.

Hi @KellenSunderland, I have added the testcase with more than 1 NDArray : )

KellenSunderland · 2019-08-07T17:40:33Z

LGTM. After considering the API a little I think I wouldn't' consider this a breaking change. Consumers of this package would have gotten errors when trying to pass lists of NDArrays to the function. It's clear that this was the intent of the API. Instead I'd consider this a patch/bugfix.

sxjscience · 2019-08-07T20:29:01Z

~~I think it looks good. But where is this API used? Is it used in some other packages?~~

Ignore this comment.
After reviewing the history I learned that it is will be used in MobulaOP

sxjscience · 2019-08-07T20:58:45Z

tests/cpp/engine/threaded_engine_test.cc

-  LOG(INFO) << "===== Test #8: PushSyncND invalid number of mutable nds =====";
-  res = MXEnginePushSyncND(FooSyncFunc, nullptr, nullptr, &ctx, nullptr, 0, &nd, -1);
-  EXPECT_EQ(res, -1);
+  std::vector<mxnet::NDArray*> nds;


In fact, I'm thinking if it's more appropriate to use

std::vector<mxnet::NDArray> nds;

Do we have to use std::vector<mxnet::NDArray*>?

We have to use std::vector<mxnet::NDArray*>, because the type of the argument of the two APIs is an array pointer of NDArray*. Besides, it avoids the potential copy of NDArray.

There is an argument Context in the constructor of NDArray. I do not know how to use std::vector<mxnet::NDArray>.

Yes, we have to use std::vector<NDArray*> if we keep the interface to be NDArrayHandle *. However, I'm thinking whether we could directly use the std::vector<mxnet::NDArray> vec; and use nds.emplace_back(mxnet::NDArray(ctx)) or nds.push_back(std::move(temp_arr)) to fill the vector. In that case, the existing API will not be changed.

It sounds good, but I am worry that the type of arguments (const_vars_handle and mutable_vars_handle) of the two APIs MXEnginePushAsync and MXEnginePushSync is EngineVarHandle, namely void*. It casts void* to VarHandle*, namely Var** in https://github.com/apache/incubator-mxnet/blob/master/src/c_api/c_api.cc#L1475. Therefore, I don't know how to decide the type of const_nds_handle and mutable_nds_handle in MXEnginePushAsyncND and MXEnginePushSyncND.

To keep the consistency, const_var_handle and mutable_var_handle is the pointer of an array of VarHandle, and const_nds_handle and mutable_var_handle is the pointer of an array of NDArrayHandle.

I now understand the logic here. To make the API consistent, I think we should also change the interface of MXEnginePushAsync and MXEnginePushSync. We should be safe to replace EngineVarHandle with VarHandle*. Am I right here ? @apeforest @yuxihu

sxjscience

LGTM. The question left is whether to change the interface of MXEnginePushAsync and MXEnginePushSync to make the API consistent.

…#15751) * fix push sync nd api * align code * update test for syncnd * fix bug in tests/cpp/engine/threaded_engine_test * add more testcases for MXEnginePushSyncND and MXEnginePushAsyncND * fix test * fix * fix * lint * ci * retrigger CI

#15792) * fix push sync nd api * align code * update test for syncnd * fix bug in tests/cpp/engine/threaded_engine_test * add more testcases for MXEnginePushSyncND and MXEnginePushAsyncND * fix test * fix * fix * lint * ci * retrigger CI

…#15751) * fix push sync nd api * align code * update test for syncnd * fix bug in tests/cpp/engine/threaded_engine_test * add more testcases for MXEnginePushSyncND and MXEnginePushAsyncND * fix test * fix * fix * lint * ci * retrigger CI

fix push sync nd api

3e8c4b5

wkcn requested review from anirudh2290 and eric-haibin-lin as code owners August 5, 2019 04:24

wkcn added Bug API change C API labels Aug 5, 2019

wkcn added 2 commits August 5, 2019 12:30

align code

8ec22bf

update test for syncnd

87db11d

wkcn mentioned this pull request Aug 5, 2019

Add TVM Bridge wkcn/MobulaOP#45

Merged

fix bug in tests/cpp/engine/threaded_engine_test

bdc47ad

wkcn commented Aug 5, 2019

View reviewed changes

KellenSunderland reviewed Aug 6, 2019

View reviewed changes

wkcn mentioned this pull request Aug 6, 2019

[Discussion] 1.5.1 Patch Release #15613

Closed

wkcn added 5 commits August 7, 2019 09:51

add more testcases for MXEnginePushSyncND and MXEnginePushAsyncND

753dc2b

fix test

290f324

fix

d34a58e

fix

85afeaf

lint

15c8b26

wkcn mentioned this pull request Aug 7, 2019

the bug of MXEnginePushAsyncND and MXEnginePushSyncND #15774

Closed

ci

14d4eb9

wkcn removed the C API label Aug 7, 2019

wkcn mentioned this pull request Aug 7, 2019

[Backport][v1.5.x] Fix the bug of MXEnginePushAsyncND and MXEnginePushSyncND #15775

Closed

7 tasks

KellenSunderland approved these changes Aug 7, 2019

View reviewed changes

sxjscience reviewed Aug 7, 2019

View reviewed changes

sxjscience approved these changes Aug 7, 2019

View reviewed changes

wkcn force-pushed the fix_push_sync_nd_api branch from ee98c12 to 14d4eb9 Compare August 7, 2019 23:25

wkcn added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Aug 7, 2019

retrigger CI

a7ae4ec

wkcn merged commit 79d8d86 into apache:master Aug 8, 2019

wkcn mentioned this pull request Aug 8, 2019

[Backport][v1.5.x] Fix the bug of MXEnginePushAsyncND and MXEnginePushSyncND #15792

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the bug of `MXEnginePushAsyncND` and `MXEnginePushSyncND` #15751

Fix the bug of `MXEnginePushAsyncND` and `MXEnginePushSyncND` #15751

wkcn commented Aug 5, 2019 •

edited

Loading

wkcn Aug 5, 2019

sxjscience commented Aug 5, 2019

wkcn commented Aug 5, 2019

wkcn commented Aug 6, 2019

KellenSunderland commented Aug 6, 2019

wkcn commented Aug 6, 2019

KellenSunderland Aug 6, 2019

wkcn Aug 7, 2019

KellenSunderland commented Aug 7, 2019

sxjscience commented Aug 7, 2019 •

edited

Loading

sxjscience Aug 7, 2019

wkcn Aug 7, 2019

wkcn Aug 7, 2019

sxjscience Aug 7, 2019

wkcn Aug 7, 2019

wkcn Aug 7, 2019

sxjscience Aug 7, 2019

sxjscience left a comment

Fix the bug of MXEnginePushAsyncND and MXEnginePushSyncND #15751

Fix the bug of MXEnginePushAsyncND and MXEnginePushSyncND #15751

Conversation

wkcn commented Aug 5, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

sxjscience commented Aug 5, 2019

wkcn commented Aug 5, 2019

wkcn commented Aug 6, 2019

KellenSunderland commented Aug 6, 2019

wkcn commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KellenSunderland commented Aug 7, 2019

sxjscience commented Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sxjscience left a comment

Choose a reason for hiding this comment

Fix the bug of `MXEnginePushAsyncND` and `MXEnginePushSyncND` #15751

Fix the bug of `MXEnginePushAsyncND` and `MXEnginePushSyncND` #15751

wkcn commented Aug 5, 2019 •

edited

Loading

sxjscience commented Aug 7, 2019 •

edited

Loading