add import_ for SymbolBlock by piiswrong · Pull Request #11127 · apache/mxnet

piiswrong · 2018-06-01T23:05:25Z

Description

(Brief description on what this PR is about)

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

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

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

marcoabreu · 2018-06-02T00:52:14Z

        prefix='resnet', ctx=ctx, pretrained=True)
    model.hybridize()
-    data = mx.nd.random.normal(shape=(1, 3, 224, 224))
+    data = mx.nd.random.normal(shape=(1, 3, 32, 32))


any specific reason for this change?

piiswrong · 2018-06-02T04:41:50Z

@ThomasDelteil

ThomasDelteil · 2018-06-04T20:41:07Z

    >>> print(feat_model(x))
    """
+    @staticmethod
+    def import_(symbol_file, input_names, param_file=None, ctx=None):


I don't think import_ is a great name, even though I understand that we want to be as close as possible to .export and import being a key-word makes it impossible to use .import().
I would suggest adding an alias to export, export_symbols export_to_symbol export_hybridized, etc and have the matching one for import. That way it would be clearer, as adding a _ suffix is not a conventionnal thing to do and is close to the use of _ as a prefix, which is for private functions.

ThomasDelteil

I think imports is a good compromise in terms of naming 👍

szha

I don't believe the save_params change is a step in the right direction.

anirudh2290 · 2018-06-07T18:49:36Z

+            present in this Block.
+        """
+        warnings.warn("load_params is deprecated. Please use load_parameters.")
+        import pdb;pdb.set_trace()


remove these pdb statements here and elsewhere ?

ThomasDelteil · 2018-06-07T19:20:25Z

Can you please update the tutorial here: https://github.com/apache/incubator-mxnet/blob/master/docs/tutorials/gluon/save_load_params.md to show the new simpler usage of .imports() ? thanks!

Just read the tutorial and noticed a few things that are wrong because of how it was iteratively updated:
L13 and L243 have references to model.load_checkpoint that are not correct and should be updated to reference the new .imports() API, if you don't mind doing it as part of this PR.

szha · 2018-06-07T20:09:42Z

+            Path to file.
+        """
+        warnings.warn("save_params is deprecated. Please use save_parameters.")
+        self.collect_params().save(filename, strip_prefix=self.prefix)


let's not make this change to the save_params.

ThomasDelteil · 2018-06-12T01:13:45Z

+        filename : str
+            Path to file.
+        """
+        warnings.warn("save_params is deprecated. Please use save_parameters.")


shall we add something about export? Something like "If you are using an hybridized model and want to serialize it to obtain the network structure and parameters, please refer to HybridBlock.export()"

save_params change has been reverted.

anirudh2290 · 2018-06-12T03:56:29Z

Wasn't the final decision to provide deprecation warning for save_params and load_params and provide new api for save_parameter and load_parameter ? Can you please explain.

szha · 2018-06-12T15:55:56Z

@anirudh2290, no, such change would break users who are on 1.2.0.

ThomasDelteil · 2018-06-12T18:11:53Z

@@ -61,7 +61,7 @@ def build_lenet(net):
        net.add(gluon.nn.Dense(512, activation="relu"))


L13 there is also "load_checkpoint and load methods" -> "imports method

we can make this change as part of another PR to avoid another round of CI.

sorry didnt realize this was already in.

* add import_ for SymbolBlock * fix * Update block.py * add save_parameters * fix * fix lint * fix * fix * fix * fix * fix * Update save_load_params.md

piiswrong requested a review from szha as a code owner June 1, 2018 23:05

marcoabreu reviewed Jun 2, 2018

View reviewed changes

ThomasDelteil reviewed Jun 4, 2018

View reviewed changes

ThomasDelteil mentioned this pull request Jun 6, 2018

Save/Load Model? #11144

Closed

piiswrong force-pushed the saveload branch from 27e3960 to d29aae1 Compare June 7, 2018 02:13

ThomasDelteil approved these changes Jun 7, 2018

View reviewed changes

szha previously requested changes Jun 7, 2018

View reviewed changes

anirudh2290 reviewed Jun 7, 2018

View reviewed changes

ThomasDelteil mentioned this pull request Jun 7, 2018

Update chapter 7 save API zackchase/mxnet-the-straight-dope#498

Merged

szha reviewed Jun 7, 2018

View reviewed changes

piiswrong force-pushed the saveload branch from 22e17a5 to 7330226 Compare June 11, 2018 06:13

ThomasDelteil reviewed Jun 12, 2018

View reviewed changes

piiswrong force-pushed the saveload branch from a54569b to d101140 Compare June 12, 2018 01:30

piiswrong and others added 11 commits June 12, 2018 10:09

add import_ for SymbolBlock

305eaca

fix

3f42282

Update block.py

d00f6fe

add save_parameters

191b173

fix

844bf2c

fix lint

2382018

fix

93f067d

fix

de4ec6e

fix

9a896ce

fix

d5b753f

fix

557a131

piiswrong force-pushed the saveload branch from d101140 to 557a131 Compare June 12, 2018 17:09

ThomasDelteil reviewed Jun 12, 2018

View reviewed changes

Update save_load_params.md

5bdf327

anirudh2290 mentioned this pull request Jun 12, 2018

[MXNET-532] Clarify documentation of save_parameters(), load_parameters() #11210

Merged

ThomasDelteil mentioned this pull request Jun 13, 2018

Symbolic .json file not compatible with .params file generated since MXNet 1.2 #11091

Closed

piiswrong merged commit 66ab27e into apache:master Jun 14, 2018

anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Jun 14, 2018

add import_ for SymbolBlock (apache#11127)

44a1734

anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Jun 15, 2018

add import_ for SymbolBlock (apache#11127)

154aa7e

anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Jun 15, 2018

add import_ for SymbolBlock (apache#11127)

78c372a

anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Jun 15, 2018

add import_ for SymbolBlock (apache#11127)

706c6ce

anirudh2290 pushed a commit that referenced this pull request Jun 15, 2018

add import_ for SymbolBlock (#11127)

719beea

		@@ -61,7 +61,7 @@ def build_lenet(net):
		net.add(gluon.nn.Dense(512, activation="relu"))

Conversation

piiswrong commented Jun 1, 2018

Description

Checklist

Essentials

Changes

Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piiswrong commented Jun 2, 2018

Uh oh!

ThomasDelteil Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasDelteil left a comment

Choose a reason for hiding this comment

Uh oh!

szha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasDelteil commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anirudh2290 commented Jun 12, 2018

Uh oh!

szha commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ThomasDelteil Jun 4, 2018 •

edited

Loading

ThomasDelteil commented Jun 7, 2018 •

edited

Loading