Fix load subgraph from json

[ZhennanQin]

For a node containing subgraphs attrs, we need to build subgraphs attrs before build attribute parser, as parser may need to access subgraphs attrs.
@zheng-da @pengzhao-intel

[wweic]

Hey, it would be great if you can add a unit test(either in python or c++). I personally prefer c++ since the test and implementation will be in the same language and it's easier to debug in the future.

[ZhennanQin]

@wweic Thanks for your comment. This is a minor change that just fixing a bug. It's not easy to create a test inside nnvm as the ops containing subgraph attrs are all inside MXNet. Also, I even don't see any tests with subgraphs, I'm not sure if this feature could be tested inside nnvm.

[wweic]

@ZhennanQin Thanks. As the contribution guide said, Add test-cases to cover the new features or bugfix the patch introduces. We should try our best to follow the guide so the code base maintain good test coverage(or growing!), and we will be more confident reviewing changes cause it's covered by unit test. Please don't get me wrong, bug fixes are great! I'm just trying to see if we can improve it further.

I think for this fix, if you have the JSON representation of such graph, you can easily call g.apply("LoadJSON") and verify the graph is correctly parsed. I believe you must have verified the fix in some way? How difficult is it to dump the problematic op into JSON? This is just some idea, feel free to ignore it if it's too much work for you. It will also help if you can share how you verify the fix in the pull request description.

[ZhennanQin]

I understand that improving test coverage is important, and it could protect your change not being broken in future. But as I said, it's difficult to add such a test inside NNVM repo, while it's much easier to add in MXNet.

For your suggestion, I could upload the json of my graph that can trigger this bug, but it would parse correctly even before merging my fix. Because the bug is caused from Cached_op parser, which will generate incorrect Cached_op params before this fix. As Cached_op isn't a part of NNVM repo, if I want to add such a test, I guess I need to create a fake Cached_op inside NNVM to simulate its parser behavior, which is a bit over work for such a small bug fix.

[wweic]

@ZhennanQin thanks for the context. That makes a lot more sense to me now.

[tqchen]

Thanks @wweic @ZhennanQin for good discussion, given the special case presented here, i will directly merge it