Fix bug with symbol loading #9573

dabraude · 2018-01-26T10:55:04Z

Description

Simplified some processes using the C API

Checklist

Essentials

[X ] Passed code style checking (make lint)
Changes are complete (i.e. I finished coding on this PR)
[ N/A ] 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
[X To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Helper functions for using the C API

Comments

Added two functions that wrap existing functionality to make it easier to use in C / C++ applications.

MXInitialise
Readies mxnet for use. While this is possible with some other commands this function makes more sense and does not require unneeded variables.

MXNDArrayLoadListFromMemory
Duplicates MXNDArrayLoad but from a location in memory. This was needed for my work as we bundle models into another file.

MXInitialise Readies mxnet for use. While this is possible with some other commands this function makes more sense and does not require unneeded variables. MXNDArrayLoadListFromMemory Duplicates MXNDArrayLoad but from a location in memory. This was needed for my work as we bundle models into another file.

marcoabreu · 2018-01-26T15:20:29Z

include/mxnet/c_api.h

+ * This ensures the engine has been initialised and
+ * the operators have been registered.
+ */
+MXNET_DLL void MXInitialise();


In which case is it required to call this method? It sounds like this should be documented widely.

Under the hood it just calls the list symbols method which registers the operators, but this means the purpose of the call is clearer. The examples all call something that calls the same method.

nit: Initialize is spelled with a 'z' :)

marcoabreu · 2018-01-26T16:03:01Z

I can see that, thanks. But that doesn't explain why it was never required and has not caused any issues so far. Please explain how you encountered a problem and add a test to cover this case. Am 26.01.2018 8:00 vorm. schrieb "David Braude" <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In include/mxnet/c_api.h <#9573 (comment)> : > @@ -201,6 +201,12 @@ MXNET_DLL const char *MXGetLastError(); * \return 0 when success, -1 when failure happens. */ MXNET_DLL int MXRandomSeed(int seed); +/*! + * \brief Initialise MXNet + * This ensures the engine has been initialised and + * the operators have been registered. + */ +MXNET_DLL void MXInitialise(); Under the hood it just calls the list symbols method which registers the operators, but this means the purpose of the call is clearer. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARxB6zXOVDm644YcG7m1292IseZ7Fh5Dks5tOfa3gaJpZM4RuJS6> .

dabraude · 2018-01-26T16:48:35Z

with just this

if you comment out the MXInitialise():

`
#include <stdlib.h>
#include <stdio.h>
#include <mxnet/c_api.h>

int main() {

mx_uint out_size, out_name_size;
NDArrayHandle *out_arr;
SymbolHandle sym;
const char ** out_names;
const char * param_fn = "checkpoint-0000.params";
const char * symbol_fn = "symbol.json";

printf("Loading mxnet\n");
MXInitialise();

MXNDArrayLoad(param_fn, &out_size, &out_arr, &out_name_size, &out_names);
MXSymbolCreateFromFile(symbol_fn, &sym);
return 0;

}
`

you will get
Check failed: op != nullptr Operator FullyConnected is not registered

marcoabreu · 2018-01-26T16:51:14Z

Thanks for the explanation!

@szha @piiswrong is this the correct usage?

piiswrong · 2018-01-26T18:04:08Z

include/mxnet/c_api.h

+ * \param out_names the names of returning NDArrays, can be NULL
+ * \return 0 when success, -1 when failure happens
+ */
+MXNET_DLL int MXNDArrayLoadListFromMemory(const void *buf,


why do we need this?

Without it it becomes difficult to put all your resources in 1 file. For instance we are a text to speech company and we need to keep all the resources for a voice in one file. One could also resolve the problem by using a file pointer instead of a buffer, but I thought this would be a more general approach.

As it would allow you to use encrypted or compressed formats too

We already have an API for loading and saving list/dict of NDArrays

In the c_api.h the only function that looked right for working from memory was MXNDArrayLoadFromRawBytes but that seems to only work with 1 array rather than loading a list/dict unless I misunderstood?

I was looking over the predict API and there are functions there that can get this data
but it requires loading the list handle, then manually creating NDArrays from each element in the list. Would it not be better to do this in one function in the main api?

Merge branch 'master' of git://github.com/apache/incubator-mxnet

dabraude · 2018-01-30T09:42:09Z

sorry I was away for a long weekend, any other comments or questions?

cjolivier01 · 2018-02-01T16:35:32Z

include/mxnet/c_api.h

+ * This ensures the engine has been initialised and
+ * the operators have been registered.
+ */
+MXNET_DLL void MXInitialise();


nit: Initialize is spelled with a 'z' :)

cjolivier01 · 2018-02-01T16:38:34Z

src/c_api/c_api.cc

+  ret->ret_handles.resize(data.size());
+  for (size_t i = 0; i < data.size(); ++i) {
+    NDArray *ptr = new NDArray();
+    *ptr = data[i];


Looks like you immediately reassign 'ptr' and leak the newly created NDArray?

Valgrind gives no errors, and if you look at MXNDArrayLoad it is the same as how files are loaded

Oh I see, it's dereferenced. Not sure how I missed that.

cjolivier01 · 2018-02-01T16:45:32Z

src/c_api/c_api.cc

@@ -91,6 +91,12 @@ int MXRandomSeed(int seed) {
  API_END();
 }

+void MXInitialise() {


What happens if this isn't called?

I put in the example code above, but the functions are not registered so if you try and load a symbol from a file before calling it you will get an error:

Check failed: op != nullptr Operator FullyConnected is not registered

So wouldn't this rather be a task to do when you load a file, rather than having this as a standalone function?

If you look at src/c_api/c_predict_api.cc lines 111
There is a bit where all the symbols are registered with MXListAllOpNames
I think it would be better to use a function whose name is clear, which can be inserted whenever it is needed such as in file loading and the prediction API.

piiswrong · 2018-02-05T20:38:39Z

The CAPI is meant to follow the hour glass model. Its like small kernel. If something can be done by composing existing API we shouldn't add a new API for it. It should be done in the frontend.

This API feels like a convenience wrapper rather than core functionality.

dabraude · 2018-02-06T10:37:23Z

so I tried to implement the equivalent to the load from memory using the predict API and the main API, but in the end I failed. I need to be able to load the main API because it is an LSTM model which the Predict API does not support. I will remove the initialise function and fix the bugs with symbol loading instead but I don't think it is actually possible to fix load a dictionary / list from memory into NDArray handles. Would you prefer me moving the loading function into a different request so the bug can be fixed quicker?

dabraude · 2018-02-13T12:56:25Z

Was at a conference, don't know where that build error was from but it seemed to fix itself once I updated to the latest version

dabraude

updated and removed extra api feature

cjolivier01 · 2018-02-20T18:20:58Z

"Approved changes" by me only refer to my comments.

… CNN network using the models defined by example/image-classification (apache#9805) * Enable the reporting of cross-entropy or nll loss value during training * Set the default value of loss as a '' to avoid a Python runtime issue when loss argument is not set

* Add SoftSign Activation function

…ripts fail fast (apache#9863)

* draft * move to contrib * rename op * CR comments * Update contrib.py * Update contrib.py * Update random.py * update example in the doc * update example in symbol doc * CR comments * update op name * update op name * update op name in test * update test * Update contrib.py

* Example implementation of the softmax output layer using RTC * Remove broken NDArrayOp example * Update README.md of the python custom operator examples

… etc. (apache#8972) * Profiling enhancements * Set new dmlc-core commit * Address CR comments * Update profiler.py * lint

* added test output files to gitignore * corrected indentation

dabraude · 2018-02-24T09:32:20Z

this has fallen way behind the master I'm going to close this pull request and do the two parts separately

marcoabreu reviewed Jan 26, 2018

View reviewed changes

piiswrong reviewed Jan 26, 2018

View reviewed changes

szha added the API change label Jan 26, 2018

update to latest version

af19a0a

Merge branch 'master' of git://github.com/apache/incubator-mxnet

Merge branch 'master' into master

dcf94e2

cjolivier01 suggested changes Feb 1, 2018

View reviewed changes

cjolivier01 reviewed Feb 1, 2018

View reviewed changes

dabraude added 2 commits February 1, 2018 17:13

changed to us spelling

5782eed

Update c_api.cc

b987833

removing the initialise function

a206dc7

dabraude changed the title ~~Added two functions to the C API~~ Fix bug with symbol loading, and added function for loading NDArrays from memory. Feb 6, 2018

dabraude added 2 commits February 6, 2018 11:10

removed initialize function and fixed bug with symbol loading

3c65037

sync with apache

32987f1

removed extra function

fdff10b

dabraude changed the title ~~Fix bug with symbol loading, and added function for loading NDArrays from memory.~~ Fix bug with symbol loading Feb 20, 2018

Update c_api.h

226755c

dabraude commented Feb 20, 2018

View reviewed changes

cjolivier01 approved these changes Feb 20, 2018

View reviewed changes

dabraude and others added 3 commits February 21, 2018 06:45

changed submodule versions to match apache

b3ed2ba

Merge branch 'master' into master

5e00a26

Improve license_header.py, add license to individual files (apache#9849)

19cc59d

juliusshufan and others added 8 commits February 24, 2018 09:27

Add CuDNN (v6) support to Jetson builds (apache#9861)

25d7efb

Softsign Activation Function (apache#9851)

751d004

* Add SoftSign Activation function

Fix broken website build, add latest version number and made build sc…

453e004

…ripts fail fast (apache#9863)

New example of custom operator using RTC (apache#9870)

cb0e4fe

* Example implementation of the softmax output layer using RTC * Remove broken NDArrayOp example * Update README.md of the python custom operator examples

Profiling enhancements, python API, vtune and chrome tracing objects,…

41f988b

… etc. (apache#8972) * Profiling enhancements * Set new dmlc-core commit * Address CR comments * Update profiler.py * lint

Fixed indentation and added test outputs to gitignore (apache#9875)

ff548b4

* added test output files to gitignore * corrected indentation

dabraude requested review from sergeykolychev, szha, thirdwing and yzhliu as code owners February 24, 2018 09:28

dabraude closed this Feb 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with symbol loading #9573

Fix bug with symbol loading #9573

dabraude commented Jan 26, 2018

marcoabreu Jan 26, 2018

dabraude Jan 26, 2018 •

edited

Loading

cjolivier01 Feb 1, 2018

marcoabreu commented Jan 26, 2018 via email

dabraude commented Jan 26, 2018

marcoabreu commented Jan 26, 2018

piiswrong Jan 26, 2018

dabraude Jan 27, 2018

dabraude Jan 27, 2018

piiswrong Jan 31, 2018

dabraude Feb 1, 2018

dabraude Feb 1, 2018 •

edited

Loading

dabraude commented Jan 30, 2018

cjolivier01 Feb 1, 2018

cjolivier01 Feb 1, 2018

dabraude Feb 1, 2018

cjolivier01 Feb 20, 2018

cjolivier01 Feb 1, 2018

dabraude Feb 1, 2018

marcoabreu Feb 1, 2018

dabraude Feb 1, 2018

piiswrong commented Feb 5, 2018 •

edited

Loading

dabraude commented Feb 6, 2018

dabraude commented Feb 13, 2018

dabraude left a comment

cjolivier01 commented Feb 20, 2018

dabraude commented Feb 24, 2018

Fix bug with symbol loading #9573

Fix bug with symbol loading #9573

Conversation

dabraude commented Jan 26, 2018

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

dabraude Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu commented Jan 26, 2018 via email

dabraude commented Jan 26, 2018

marcoabreu commented Jan 26, 2018

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

dabraude Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

dabraude commented Jan 30, 2018

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

Choose a reason for hiding this comment

piiswrong commented Feb 5, 2018 • edited Loading

dabraude commented Feb 6, 2018

dabraude commented Feb 13, 2018

dabraude left a comment

Choose a reason for hiding this comment

cjolivier01 commented Feb 20, 2018

dabraude commented Feb 24, 2018

dabraude Jan 26, 2018 •

edited

Loading

dabraude Feb 1, 2018 •

edited

Loading

piiswrong commented Feb 5, 2018 •

edited

Loading