Infer dtype in SymbolBlock import from input symbol #12412

sandeep-krishnamurthy · 2018-08-30T16:51:26Z

Description

Fix for the issue - gluon.SymbolBlock cannot imports resnet trained with dtype="float16" #11849
Currently, Gluon symbol block cannot import any symbol with type other than fp32. All the parameters are created as FP32 leading to failure in importing the params when it is of type fp16, fp64 etc,
In this PR, we infer the type of the symbol being imported and create the Symbol Block Parameters with that inferred type.

~~Created this PR for getting the early feedback. I am working on adding test cases for this and will update the PR soon~~
Added the tests

@szha @hetong007 @apeforest - Can you please take a look at this?
@Roshrini - I think this is important fix to be picked for 1.3. @szha ?

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

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

In this PR, we infer the type of the symbol being imported and create the Symbol Block Parameters with that inferred type.
If we cannot infer the type, we go by usual route using default dtype (fp32) for creating the parameters.

Comments

Below is the issue:

When you create mx.gluon.SymbolBlock(sm, input). It creates the parameters in the Block. Type is not passed for creating the parameter.
See here - https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/gluon/block.py#L1058 and the behavior is "If there is not parameter to get, it creates one and uses default type (fp32) https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/gluon/parameter.py#L688

apeforest

Left a few small comments. Otherwise looks great. Thanks for the quick fix!

apeforest · 2018-08-31T16:32:51Z

python/mxnet/gluon/block.py

+                    self.params.get(aux, grad_req='null', allow_deferred_init=True, dtype=aux_types[i])
+        else:
+            # Use default types for params
+            for i, arg in enumerate(arg_params):


Maybe no enumerate is needed since you don't use index i in this loop?

apeforest · 2018-08-31T16:33:14Z

python/mxnet/gluon/block.py

-        for i in out.list_auxiliary_states():
-            if i not in input_names:
-                self.params.get(i, grad_req='null', allow_deferred_init=True)
+            for i, aux in enumerate(aux_params):


Same here, no need to get index i

N/A with latest refactoring.

python/mxnet/gluon/block.py

lupesko

Suggest to add a test for float16 as well as float64

lupesko · 2018-08-31T18:15:55Z

tests/python/unittest/test_gluon.py

+    ctx = mx.cpu(0)
+
+    net_fp32 = mx.gluon.model_zoo.vision.resnet34_v2(pretrained=True, ctx=ctx)
+    net_fp32.cast('float64')


Shall we add another test that casts to float16?
The original issue reported import from float16 failing, and it might be appropriate to cover it as well.

Added a test for fp16.

sandeep-krishnamurthy · 2018-08-31T20:01:38Z

@apeforest @szha @lupesko - Thanks for your time. Addressed your comments. Please have a look.

…ndition

apeforest · 2018-08-31T21:33:54Z

tests/python/unittest/test_gluon.py

+    tmpfile = os.path.join(tmp, 'resnet34_fp64')
+    ctx = mx.cpu(0)
+
+    net_fp32 = mx.gluon.model_zoo.vision.resnet34_v2(pretrained=True, ctx=ctx, root=tmp)


Is the model name in model zoo going to be maintained? If there is any change in the name, it would break this unit test. Not sure if we want to keep this dependency

resnet34_v2 is a public function exposed through model_zoo.vision module. I think it is ok because we are not using string based selection of the model.

sandeep-krishnamurthy · 2018-09-04T18:49:48Z

@szha @apeforest - Is this good to go?

apeforest

LGTM

stu1130 · 2018-09-07T03:02:23Z

tests/python/unittest/test_gluon.py

+    # other than fp32 param dtype.
+
+    # 1. Load a resnet model, cast it to fp64 and export
+    tmp = tempfile.mkdtemp()


should we delete the temporary directory when done with it?

Temp gets automatically cleaned up.

According to python docs, it seems that it should be deleted by the user. The user of mkdtemp() is responsible for deleting the temporary directory and its contents when done with it.

Right. Thanks. I meant, temp gets automatically cleaned up after all the tests (test session).

@marcoabreu - Can you please confirm if my understanding is correct? If not, I will add code to delete the temp directory created in the tests. Also, I see similar behavior in all other tests, where it creates temp dir, but, assumes, it will be cleaned up the system.

sandeep-krishnamurthy · 2018-09-10T18:53:41Z

@zhreshold - Can you help look at this once, and merge if looks good?

zhreshold · 2018-09-10T19:08:25Z

python/mxnet/gluon/block.py

-        for i in out.list_auxiliary_states():
-            if i not in input_names:
-                self.params.get(i, grad_req='null', allow_deferred_init=True)
+        arg_types, aux_types = _infer_param_types(inputs[0], out, arg_params, aux_params)


this does not handle Grouped Symbol because you are only slicing [0] from the symbol.

also, I think the type inference should occur in cast as well, otherwise it's buggy when user is trying to cast dtype of a cascaded network with symbolBlock inside.

zhreshold · 2018-09-10T19:56:03Z

python/mxnet/gluon/block.py

-        for i in out.list_auxiliary_states():
-            if i not in input_names:
-                self.params.get(i, grad_req='null', allow_deferred_init=True)
+        arg_types, aux_types = _infer_param_types(inputs[0], out, arg_params, aux_params)


also, I think the type inference should occur in cast as well, otherwise it's buggy when user is trying to cast dtype of a cascaded network with symbolBlock inside.

sandeep-krishnamurthy · 2018-09-12T03:06:39Z

tests/python/unittest/test_gluon.py

+            break
+    assert np.dtype(net_fp64.params[param_name].dtype) == np.dtype(np.float64)
+
+    # Cast the symbol block to FP32 and try to forward a FP32 data.


@zhreshold - Added a test here to verify the SymbolBlock.cast functionality.

sandeep-krishnamurthy · 2018-09-17T21:40:09Z

@zhreshold - Can you please take a look at this? Thanks.

python/mxnet/gluon/block.py

* Infer dtype in SymbolBlock import from input symbol * Fix lint issues and make existing tests pass * Add tests for importing a fp64 model into symbol block * Fixing failing test for test symbol block * Set context in unit tests * Add tests for fp16, add default dtype in infer_param_types * Use tmp directory as root for loading from model zoo to avoid race condition * Fixing naming and parameter selection in test case * Fixing failing GPU tests * Make unit test more deterministic to get param name * Override cast in symbol block, handle grouped symbol * Handle multiple symbolic input usecase * Add tests to verify behavior of SymbolBlock.cast

Infer dtype in SymbolBlock import from input symbol

d69b7f6

sandeep-krishnamurthy requested a review from szha as a code owner August 30, 2018 16:51

sandeep-krishnamurthy added Gluon pr-awaiting-review PR is waiting for code review pr-work-in-progress PR is still work in progress labels Aug 30, 2018

sandeep-krishnamurthy added 2 commits August 30, 2018 11:12

Fix lint issues and make existing tests pass

f82344d

Add tests for importing a fp64 model into symbol block

980c9e3

sandeep-krishnamurthy changed the title ~~[WIP] Infer dtype in SymbolBlock import from input symbol~~ Infer dtype in SymbolBlock import from input symbol Aug 30, 2018

sandeep-krishnamurthy removed the pr-work-in-progress PR is still work in progress label Aug 30, 2018

sandeep-krishnamurthy added 2 commits August 30, 2018 17:44

Fixing failing test for test symbol block

62acade

Set context in unit tests

0c4e45b

apeforest reviewed Aug 31, 2018

View reviewed changes

szha reviewed Aug 31, 2018

View reviewed changes

python/mxnet/gluon/block.py Outdated Show resolved Hide resolved

lupesko reviewed Aug 31, 2018

View reviewed changes

Add tests for fp16, add default dtype in infer_param_types

f66f7a8

Use tmp directory as root for loading from model zoo to avoid race co…

2033347

…ndition

apeforest reviewed Aug 31, 2018

View reviewed changes

sandeep-krishnamurthy added 2 commits August 31, 2018 14:37

Fixing naming and parameter selection in test case

346632d

Fixing failing GPU tests

db83669

sandeep-krishnamurthy mentioned this pull request Sep 4, 2018

float64 data backward error using gluon #9156

Closed

Make unit test more deterministic to get param name

05287e5

apeforest approved these changes Sep 4, 2018

View reviewed changes

stu1130 reviewed Sep 7, 2018

View reviewed changes

zhreshold reviewed Sep 10, 2018

View reviewed changes

zhreshold suggested changes Sep 10, 2018

View reviewed changes

sandeep-krishnamurthy added 2 commits September 11, 2018 12:37

Merge remote-tracking branch 'upstream/master' into fix_gluon_fp16

f382183

Override cast in symbol block, handle grouped symbol

fb6158a

sandeep-krishnamurthy added 2 commits September 11, 2018 16:17

Handle multiple symbolic input usecase

0ff23f8

Add tests to verify behavior of SymbolBlock.cast

b58ea41

sandeep-krishnamurthy commented Sep 12, 2018

View reviewed changes

zhreshold reviewed Sep 17, 2018

View reviewed changes

python/mxnet/gluon/block.py Show resolved Hide resolved

zhreshold approved these changes Sep 18, 2018

View reviewed changes

sandeep-krishnamurthy merged commit acf309e into apache:master Sep 18, 2018

lebeg mentioned this pull request Nov 5, 2018

Infer dtype in SymbolBlock import from input symbol (v1.3.x) #13117

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer dtype in SymbolBlock import from input symbol #12412

Infer dtype in SymbolBlock import from input symbol #12412

sandeep-krishnamurthy commented Aug 30, 2018 •

edited

Loading

apeforest left a comment

apeforest Aug 31, 2018

apeforest Aug 31, 2018

sandeep-krishnamurthy Aug 31, 2018

lupesko left a comment

lupesko Aug 31, 2018

sandeep-krishnamurthy Aug 31, 2018

sandeep-krishnamurthy commented Aug 31, 2018

apeforest Aug 31, 2018

sandeep-krishnamurthy Aug 31, 2018

sandeep-krishnamurthy commented Sep 4, 2018

apeforest left a comment

stu1130 Sep 7, 2018

sandeep-krishnamurthy Sep 7, 2018

stu1130 Sep 7, 2018 •

edited

Loading

sandeep-krishnamurthy Sep 7, 2018

sandeep-krishnamurthy Sep 7, 2018

sandeep-krishnamurthy commented Sep 10, 2018

zhreshold Sep 10, 2018

zhreshold Sep 10, 2018

zhreshold Sep 10, 2018

sandeep-krishnamurthy Sep 12, 2018

sandeep-krishnamurthy commented Sep 17, 2018

Infer dtype in SymbolBlock import from input symbol #12412

Infer dtype in SymbolBlock import from input symbol #12412

Conversation

sandeep-krishnamurthy commented Aug 30, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

apeforest left a 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

lupesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Sep 4, 2018

apeforest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu1130 Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Sep 10, 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

sandeep-krishnamurthy commented Sep 17, 2018

sandeep-krishnamurthy commented Aug 30, 2018 •

edited

Loading

stu1130 Sep 7, 2018 •

edited

Loading