You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/10/23 06:58:42 UTC

[GitHub] [incubator-mxnet] leezu opened a new pull request #19417: NumPy compatible serialization API

leezu opened a new pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417


   ## Description ##
   Adopt NumPy and SciPy serialization formats.
   See https://docs.scipy.org/doc/numpy-1.10.4/neps/npy-format.html for details.
   
   Fixes https://github.com/apache/incubator-mxnet/issues/18667
   Loading legacy parameter formats is still supported. Saving legacy parameter formats is no longer exposed to users.
   
   WIP:
   - Load & save sparse array like https://docs.scipy.org/doc/scipy/reference/generated/scipy.sparse.save_npz.html
   
   Features that won't be supported by this PR and may be implemented later
   
   - Save / load npz format from cloud storage (S3, Azure, ...)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520870302



##########
File path: ci/docker/Dockerfile.build.arm
##########
@@ -64,13 +64,25 @@ RUN curl -o armv6-eabihf--glibc--stable-2020.02-2.tar.bz2 -L https://toolchains.
     rm armv6-eabihf--glibc--stable-2020.02-2.tar.bz2
 ENV CMAKE_TOOLCHAIN_FILE=/usr/local/armv6-eabihf--glibc--stable-2020.02-2/share/buildroot/toolchainfile.cmake
 
-RUN git clone --recursive -b v0.3.9 https://github.com/xianyi/OpenBLAS.git && \
+RUN git clone --recursive -b v0.3.12 https://github.com/xianyi/OpenBLAS.git && \

Review comment:
       I think it's ok to upgrade as long as there's enough confidence in the new version. up to you




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-729343714


   @mxnet-bot run ci [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520853432



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1381,12 +1381,12 @@ def export(self, path, epoch=0, remove_amp_cast=True):
                                       .format(name=name), stacklevel=3)
                     else:
                         arg_dict['aux:%s'%name] = param._reduce()
-        save_fn = _mx_npx.save if is_np_array() else ndarray.save
+        save_fn = _mx_npx.savez if is_np_array() else ndarray.save

Review comment:
       npy and npz formats are numpy formats. We can serialize legacy mxnet arrays to the new (more general) numpy semantics, but not necessarily the other way around. What is your question?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-727148073


   The build-time failure on gpu windows builds is resolved. Only the "access violation writing" on windows remains.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-730153832


   Jenkins CI successfully triggered : [centos-gpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu edited a comment on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu edited a comment on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-726484091


   @yajiedesign @zhreshold any idea why this fails on Windows-CPU with
   ```
   [2020-11-13T01:24:44.124Z] >       check_call(_LIB.MXNDArraySave(c_str(file), mx_uint(len(handles)), handles, keys))
   [2020-11-13T01:24:44.124Z] E       OSError: exception: access violation writing 0xFFFFFFFFAAA338B0
   ```
   when opening / saving a zip file via libzip?
   
   Also any idea why on the Windows GPU builds the include directories get lost and we obtain compile error like
   
   ```
   [2020-11-12T02:47:08.659Z] ..\src\c_api\c_api.cc(63): fatal error C1083: Cannot open include file: 'zip.h': No such file or directory
   ```
   
   even though there is no issue finding the include files on Windows CPU build?
   
   Sholud we leave Windows builds for now to re-use the old MXNet serialization format and fix the numpy compatible serialization later once the Windows toolchain is more stable?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r511619968



##########
File path: docs/python_docs/python/tutorials/packages/np/np-vs-numpy.md
##########
@@ -91,16 +91,20 @@ b = a[0]
 b.ndim, b.size, isinstance(b, np.ndarray)
 ```
 
-## Save
+## Save and load
 
-The `save` method in `mxnet.np` saves data into a binary format that's not compatible with NumPy format. For example, it contains the device information. (TODO, needs more discussion here.) 
+Users should call the official NumPy `save` and `load` methods respectively to save and load arrays.
 
 ```{.python .input}
 a = np.array(1, ctx=gpu)
-npx.save('a', a)
-npx.load('a')
+onp.save('a', a)
+onp.load('a.npy', a)
 ```
 
+For advanced use-cases, MXNet further provides `npx.save` and `npx.load`, which
+can save and load a dictionary of arrays to the `.npz` format such that it can

Review comment:
       should we follow the savez naming convention?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-730479443


   @mxnet-bot run ci [centos-gpu, windows-gpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-726484091


   @yajiedesign @zhreshold any idea why this fails on Windows-CPU with
   ```
   [2020-11-13T01:24:44.124Z] >       check_call(_LIB.MXNDArraySave(c_str(file), mx_uint(len(handles)), handles, keys))
   [2020-11-13T01:24:44.124Z] E       OSError: exception: access violation writing 0xFFFFFFFFAAA338B0
   ```
   when opening / saving a zip file via libzip?
   
   Also any idea why on the Windows GPU builds the include directories get lost and we obtain compile error like
   
   ```
   [2020-11-12T02:47:08.659Z] ..\src\c_api\c_api.cc(63): fatal error C1083: Cannot open include file: 'zip.h': No such file or directory
   ```
   
   even though there is no issue finding the include files on Windows CPU build?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520190161



##########
File path: docs/python_docs/python/tutorials/packages/np/np-vs-numpy.md
##########
@@ -91,16 +91,20 @@ b = a[0]
 b.ndim, b.size, isinstance(b, np.ndarray)
 ```
 
-## Save
+## Save and load
 
-The `save` method in `mxnet.np` saves data into a binary format that's not compatible with NumPy format. For example, it contains the device information. (TODO, needs more discussion here.) 
+Users should call the official NumPy `save` and `load` methods respectively to save and load arrays.
 
 ```{.python .input}
 a = np.array(1, ctx=gpu)
-npx.save('a', a)
-npx.load('a')
+onp.save('a', a)
+onp.load('a.npy', a)
 ```
 
+For advanced use-cases, MXNet further provides `npx.save` and `npx.load`, which
+can save and load a dictionary of arrays to the `.npz` format such that it can

Review comment:
       Added `npx.savez` that can save multiple arrays (dense or sparse) to the npz format. Updated `npx.save` to only support saving a single dense array to `.npy`.
   
   I'm keeping them in the `npx` namespace as we do not support pickling objects etc.
   
   WDYT?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520719900



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1381,12 +1381,12 @@ def export(self, path, epoch=0, remove_amp_cast=True):
                                       .format(name=name), stacklevel=3)
                     else:
                         arg_dict['aux:%s'%name] = param._reduce()
-        save_fn = _mx_npx.save if is_np_array() else ndarray.save
+        save_fn = _mx_npx.savez if is_np_array() else ndarray.save

Review comment:
       does the parameter class note down whether it was created as np array or legacy ndarray? how are we handling attempts at sharing parameters between the two?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-730479514


   Jenkins CI successfully triggered : [windows-gpu, centos-gpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r526581655



##########
File path: src/c_api/c_api.cc
##########
@@ -1899,6 +1901,36 @@ int MXNDArraySave(const char* fname,
   API_END();
 }
 
+int MXNDArraySave(const char* fname,
+                  uint32_t num_args,
+                  NDArrayHandle* args,
+                  const char** keys) {
+  API_BEGIN();
+
+  CHECK_NOTNULL(fname);

Review comment:
       Modifying the message isn't supported as the macro is equal to an identity function if the argument is not null. The macro already defines an error message for the case of null arguments.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-730153801


   @mxnet-bot run ci [centos-gpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-715012538


   Hey @leezu , Thanks for submitting the PR 
   All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands: 
   - To trigger all jobs: @mxnet-bot run ci [all] 
   - To trigger specific jobs: @mxnet-bot run ci [job1, job2] 
   *** 
   **CI supported jobs**: [clang, centos-gpu, website, centos-cpu, windows-cpu, unix-gpu, edge, miscellaneous, sanity, windows-gpu, unix-cpu]
   *** 
   _Note_: 
    Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin. 
   All CI tests must pass before the PR can be merged. 
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r526580067



##########
File path: src/c_api/c_api.cc
##########
@@ -1899,6 +1901,36 @@ int MXNDArraySave(const char* fname,
   API_END();
 }
 
+int MXNDArraySave(const char* fname,
+                  uint32_t num_args,
+                  NDArrayHandle* args,
+                  const char** keys) {
+  API_BEGIN();
+
+  CHECK_NOTNULL(fname);

Review comment:
       add meaningful error message?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520852341



##########
File path: ci/docker/Dockerfile.build.arm
##########
@@ -64,13 +64,25 @@ RUN curl -o armv6-eabihf--glibc--stable-2020.02-2.tar.bz2 -L https://toolchains.
     rm armv6-eabihf--glibc--stable-2020.02-2.tar.bz2
 ENV CMAKE_TOOLCHAIN_FILE=/usr/local/armv6-eabihf--glibc--stable-2020.02-2/share/buildroot/toolchainfile.cmake
 
-RUN git clone --recursive -b v0.3.9 https://github.com/xianyi/OpenBLAS.git && \
+RUN git clone --recursive -b v0.3.12 https://github.com/xianyi/OpenBLAS.git && \

Review comment:
       Only as the PR is modifying the file already. Why do you think it's unstable?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520704971



##########
File path: ci/docker/Dockerfile.build.arm
##########
@@ -64,13 +64,25 @@ RUN curl -o armv6-eabihf--glibc--stable-2020.02-2.tar.bz2 -L https://toolchains.
     rm armv6-eabihf--glibc--stable-2020.02-2.tar.bz2
 ENV CMAKE_TOOLCHAIN_FILE=/usr/local/armv6-eabihf--glibc--stable-2020.02-2/share/buildroot/toolchainfile.cmake
 
-RUN git clone --recursive -b v0.3.9 https://github.com/xianyi/OpenBLAS.git && \
+RUN git clone --recursive -b v0.3.12 https://github.com/xianyi/OpenBLAS.git && \

Review comment:
       any reason to upgrade the openblas in this PR? looks like openblas hasn't been stable for the past few releases.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520887731



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1381,12 +1381,12 @@ def export(self, path, epoch=0, remove_amp_cast=True):
                                       .format(name=name), stacklevel=3)
                     else:
                         arg_dict['aux:%s'%name] = param._reduce()
-        save_fn = _mx_npx.save if is_np_array() else ndarray.save
+        save_fn = _mx_npx.savez if is_np_array() else ndarray.save

Review comment:
       Sharing parameters between models expected ndarray and NDArray may not be supported. This would trigger error at model runtime as numpy operator expect ndarray but may get NDArray.
   
   With respect to the `nd.save` function. It calls the same C-API as the prior implementation of `npx.save`. Thus saving ndarrays via `nd.save` works (as ndarray is a subclass of NDArray)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r512078377



##########
File path: src/c_api/c_api.cc
##########
@@ -1877,20 +1879,22 @@ int MXNDArraySave(const char* fname,
                   NDArrayHandle* args,
                   const char** keys) {
   API_BEGIN();
-  std::vector<NDArray> data(num_args);
-  std::vector<std::string> names;
-  for (uint32_t i = 0; i < num_args; ++i) {
-    data[i] = *static_cast<NDArray*>(args[i]);
-  }
-  if (keys != nullptr) {
-    names.resize(num_args);
-    for (uint32_t i = 0; i < num_args; ++i) {
-      names[i] = keys[i];
-    }
-  }
-  {
-    std::unique_ptr<dmlc::Stream> fo(dmlc::Stream::Create(fname, "w"));
-    mxnet::NDArray::Save(fo.get(), data, names);
+
+  CHECK_NOTNULL(fname);
+
+  if (num_args == 1 && static_cast<NDArray *>(args[0])->storage_type() == kDefaultStorage) {
+      NDArray *array = static_cast<NDArray *>(args[0]);
+      npy::save_array(fname, *array);
+  } else {
+      int write_mode = ZIP_TRUNCATE | ZIP_CREATE;
+      for (uint32_t i = 0; i < num_args; ++i) {
+          NDArray *array = static_cast<NDArray *>(args[i]);
+          const std::string array_key = keys == nullptr ? std::to_string(i) : keys[i];
+          npz::save_array(write_mode, fname, array_key, *array);

Review comment:
       yes




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-729220150


   Access violation was due to MXNet building with `WIN32_LEAN_AND_MEAN` option on Windows.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu merged pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520885048



##########
File path: ci/docker/Dockerfile.build.arm
##########
@@ -64,13 +64,25 @@ RUN curl -o armv6-eabihf--glibc--stable-2020.02-2.tar.bz2 -L https://toolchains.
     rm armv6-eabihf--glibc--stable-2020.02-2.tar.bz2
 ENV CMAKE_TOOLCHAIN_FILE=/usr/local/armv6-eabihf--glibc--stable-2020.02-2/share/buildroot/toolchainfile.cmake
 
-RUN git clone --recursive -b v0.3.9 https://github.com/xianyi/OpenBLAS.git && \
+RUN git clone --recursive -b v0.3.12 https://github.com/xianyi/OpenBLAS.git && \

Review comment:
       The note only applies to 0.3.11. 0.3.12 should be fine. I think we should update to ensure that we do test the newer versions of OpenBLAS (the old version is tested on CentOS). I can open a separate PR for it if you prefer.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520869334



##########
File path: ci/docker/Dockerfile.build.arm
##########
@@ -64,13 +64,25 @@ RUN curl -o armv6-eabihf--glibc--stable-2020.02-2.tar.bz2 -L https://toolchains.
     rm armv6-eabihf--glibc--stable-2020.02-2.tar.bz2
 ENV CMAKE_TOOLCHAIN_FILE=/usr/local/armv6-eabihf--glibc--stable-2020.02-2/share/buildroot/toolchainfile.cmake
 
-RUN git clone --recursive -b v0.3.9 https://github.com/xianyi/OpenBLAS.git && \
+RUN git clone --recursive -b v0.3.12 https://github.com/xianyi/OpenBLAS.git && \

Review comment:
       The following note appeared in the second last patch version.
   `NOTE there appear to be several defects in this version unfortunately - this should not be redistributed or used in a production environment`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r511620726



##########
File path: src/c_api/c_api.cc
##########
@@ -1877,20 +1879,22 @@ int MXNDArraySave(const char* fname,
                   NDArrayHandle* args,
                   const char** keys) {
   API_BEGIN();
-  std::vector<NDArray> data(num_args);
-  std::vector<std::string> names;
-  for (uint32_t i = 0; i < num_args; ++i) {
-    data[i] = *static_cast<NDArray*>(args[i]);
-  }
-  if (keys != nullptr) {
-    names.resize(num_args);
-    for (uint32_t i = 0; i < num_args; ++i) {
-      names[i] = keys[i];
-    }
-  }
-  {
-    std::unique_ptr<dmlc::Stream> fo(dmlc::Stream::Create(fname, "w"));
-    mxnet::NDArray::Save(fo.get(), data, names);
+
+  CHECK_NOTNULL(fname);
+
+  if (num_args == 1 && static_cast<NDArray *>(args[0])->storage_type() == kDefaultStorage) {
+      NDArray *array = static_cast<NDArray *>(args[0]);
+      npy::save_array(fname, *array);
+  } else {
+      int write_mode = ZIP_TRUNCATE | ZIP_CREATE;
+      for (uint32_t i = 0; i < num_args; ++i) {
+          NDArray *array = static_cast<NDArray *>(args[i]);
+          const std::string array_key = keys == nullptr ? std::to_string(i) : keys[i];
+          npz::save_array(write_mode, fname, array_key, *array);

Review comment:
       does it handle non-default storage type?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#discussion_r520868839



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1381,12 +1381,12 @@ def export(self, path, epoch=0, remove_amp_cast=True):
                                       .format(name=name), stacklevel=3)
                     else:
                         arg_dict['aux:%s'%name] = param._reduce()
-        save_fn = _mx_npx.save if is_np_array() else ndarray.save
+        save_fn = _mx_npx.savez if is_np_array() else ndarray.save

Review comment:
       I'm trying to see if there will ever be the case that the behavior of the quoted statement is wrong. One case I can think of is that parameters are created with the numpy class while the flag of `is_np_array()` returns false for some reason. I imagine this is only possible when there's mixed use such as in parameter sharing, hence the question.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19417: NumPy compatible serialization API

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19417:
URL: https://github.com/apache/incubator-mxnet/pull/19417#issuecomment-729343738


   Jenkins CI successfully triggered : [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org