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/07/17 23:26:19 UTC
[GitHub] [incubator-mxnet] leezu opened a new pull request #18749: Refactor Gluon parameter serialization format
leezu opened a new pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749
Switch to npz serialization format https://numpy.org/devdocs/reference/generated/numpy.lib.format.html
At this point in time, only Python bindings exist and no functionality is introduced to load the npz files in the backend (as it's not needed). Once the need arises, bindings can be introduced with ease via https://github.com/leezu/cnpy/tree/libzip
Fixes https://github.com/apache/incubator-mxnet/issues/18717 https://github.com/apache/incubator-mxnet/issues/18667
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660439938
I think we can delete `mx.npx.load` (or make it private to have a transition period). This PR marks it as deprecated. Do you see any problems?
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660434713
There's no code in the backend that interacts with parameters. You can search for "MXNDArrayLoad" and you see that all code invoking this API has been removed. As the backend does not interact with the format, there is no point in adding APIs here, as their use is not yet defined.
The zipfile and numpy formats are trivial to work with, so there's no issue in adding the support at any time.
> Please add test for backward compatibility
The old format is faulty and I don't see a strong reason to provide backwards compatibility throughout 2.x series. We may remove the C API for loading the old format in a later PR (for example when removing the ndarray operators). As of this PR, backwards compatibility provided as there is no change to C APIs and the Python code backs of to using the C API if the input is not of the new format. It's tested via the model-zoo API.
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660434713
There's no code in the backend that interacts with parameters. You can search for "MXNDArrayLoad" and you see that all code invoking this API has been removed. As the backend does not interact with the format, there is no point in adding APIs here, as their use is not yet defined.
The zipfile and numpy formats are trivial to work with, so there's no issue in adding the support at any time.
> Please add test for backward compatibility
The old format is faulty and I don't see a strong reason to provide backwards compatibility throughout 2.x series. We may remove the C API for loading the old format in a later PR (for example when removing the ndarray operators). As of this PR, backwards compatibility provided as there is no change to C APIs and the Python code backs of to using the C API if the input is not of the new format. It's tested already via unittests and integration tests (via the model-zoo API). Unittest:
https://github.com/apache/incubator-mxnet/blob/a4ea4a8330251dd244947074cf4ddb875e611dd5/tests/python/unittest/test_ndarray.py#L1988-L2001
----------------------------------------------------------------
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 pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660577794
> their use is not yet defined
Frontend for JVM is planned in #17783. We will at least need to support inference with backend API which will require loading the parameters. MXNet will not be a Python-only framework and the design decision now on the serialization affects other frontends, so it must be taken with care.
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660379339
@ZheyuYe please verify if this addresses https://github.com/apache/incubator-mxnet/issues/18717
----------------------------------------------------------------
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] ZheyuYe commented on pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
ZheyuYe commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660439727
Yes, this PR do slove #18717. We might also need do refactor `mx.npx.load` to load 'npz' and return a parameters dict. What do you think?
----------------------------------------------------------------
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] Kh4L commented on a change in pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#discussion_r456731407
##########
File path: python/mxnet/gluon/block.py
##########
@@ -336,41 +337,43 @@ def _collect_params_with_prefix(self, prefix='', select=None):
ret.update(child()._collect_params_with_prefix(prefix + name, select))
return ret
- def save_parameters(self, filename, deduplicate=False):
- """Save parameters to file.
+ def save_parameters(self, filename):
+ """Save parameters to file based on numpy .npz format.
- Saved parameters can only be loaded with `load_parameters`. Note that this
- method only saves parameters, not model structure. If you want to save
- model structures, please use :py:meth:`HybridBlock.export`.
+ Saved parameters can be loaded with `load_parameters` or numpy.load.
+ Note that this method only saves parameters, not model structure. If
+ you want to save model structures, please use :py:meth:`HybridBlock.export`.
Parameters
----------
- filename : str
- Path to file.
- deduplicate : bool, default False
- If True, save shared parameters only once. Otherwise, if a Block
- contains multiple sub-blocks that share parameters, each of the
- shared parameters will be separately saved for every sub-block.
+ filename : str or file
+ Either the filename (string) or an open file (file-like object)
+ where the data will be saved.
References
----------
`Saving and Loading Gluon Models \
<https://mxnet.apache.org/api/python/docs/tutorials/packages/gluon/blocks/save_load_params.html>`_
- """
- params = self._collect_params_with_prefix()
-
- if deduplicate:
- # Shared parameters are stored only a single time as of MXNet 1.6.
- # Shared parameters are registered under multiple prefixes returned by
- # _collect_params_with_prefix. We select a single one and only store
- # it. In load_parameters it is sufficient for a shared parameter to
- # only set it for a single prefix.
- reverse_params = {v: k for k, v in params.items()}
- params = {v: k for k, v in reverse_params.items()}
- arg_dict = {key: val._reduce() for key, val in params.items()}
- save_fn = _mx_npx.save if is_np_array() else ndarray.save
- save_fn(filename, arg_dict)
+ """
+ params_to_names = {}
+ for name, param in self._collect_params_with_prefix().items():
+ params_to_names.setdefault(param, []).append(name)
+ params = {}
+ for param, names in params_to_names.items():
+ assert len(names)
+ params[names[0]] = param._reduce().asnumpy()
+ for name in names[1:]:
+ # Shared parameters are known under multiple names. We save the
+ # parameter according to it's first name and save the mapping
Review comment:
Typo nit: `its`
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660439938
I think we can delete `mx.npx.load` (or make it private to have a transition period)
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-662120712
I'll close this and proceed with a PR of larger scope later.
----------------------------------------------------------------
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 pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-661285270
Discussed offline and the conclusions are:
- we will go forward with this PR to allow serialization of parameters without duplicates.
- we will take into account serialization of symbol graphs in the same format later on, potentially under a different file name/extension. serialization of graph is optional and we need to support serializing parameters alone.
- @leezu will post an RFC to summarize the serialization use cases in frontends and backend.
----------------------------------------------------------------
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 pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660419990
Also, I'd like to see backend serialization/deserialization happen since it likely will be needed. If we merge this for python only first and there's any issue in adding that support later we won't be able to rollback.
----------------------------------------------------------------
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 #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749#issuecomment-660379004
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**: [centos-gpu, clang, sanity, unix-cpu, windows-cpu, website, centos-cpu, windows-gpu, miscellaneous, unix-gpu, edge]
***
_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] leezu closed pull request #18749: Refactor Gluon parameter serialization format
Posted by GitBox <gi...@apache.org>.
leezu closed pull request #18749:
URL: https://github.com/apache/incubator-mxnet/pull/18749
----------------------------------------------------------------
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