search

dmlc / gluon-nlp

NLP made easy
https://nlp.gluon.ai/
Apache License 2.0
2.56k stars 538 forks source link

bertpass_gpu.cc does not support MXNet 1.8 #1388

Open leezu opened 3 years ago

leezu commented 3 years ago

Due to API change https://github.com/apache/incubator-mxnet/issues/19135

samskalicky commented 3 years ago

Thanks @leezu. The custom pass in https://github.com/dmlc/gluon-nlp/blob/v0.10.x/scripts/bert/bertpass_gpu.cc uses some custom code that @MoisesHer and I worked on post-1.7.0 release that eventually became the default in 1.8.0. Thanks to this collaboration, in 1.8 all lines 30-248 have been integration into lib_api.h so they can be removed from bertpass_gpu.cc.

Plus, we eliminated the extra step of taking the symbol_json string and converting it to a graph object in https://github.com/dmlc/gluon-nlp/blob/3fbe9619d9e68bc665f73c8cdf683213c6edd4d6/scripts/bert/bertpass_gpu.cc#L251-L262 So now the API will be simplified to: https://github.com/apache/incubator-mxnet/blob/cc4b8ec68b6ec9dae73046e9c34ac97439efda83/example/extensions/lib_pass/pass_lib.cc#L34-L35

MXReturnValue myPass(mxnet::ext::Graph *g,
                     const std::unordered_map<std::string, std::string>& options) {

So little things like changing how you loop over the nodes in the graph from: https://github.com/dmlc/gluon-nlp/blob/3fbe9619d9e68bc665f73c8cdf683213c6edd4d6/scripts/bert/bertpass_gpu.cc#L266 will need to be changed to: https://github.com/apache/incubator-mxnet/blob/cc4b8ec68b6ec9dae73046e9c34ac97439efda83/example/extensions/lib_subgraph/subgraph_lib.cc#L308-L309

  for(int i=0; i<graph->size(); i++) {
    mxnet::ext::Node* n = graph->getNode(i);

And creating new nodes from: https://github.com/dmlc/gluon-nlp/blob/3fbe9619d9e68bc665f73c8cdf683213c6edd4d6/scripts/bert/bertpass_gpu.cc#L280 to: https://github.com/apache/incubator-mxnet/blob/cc4b8ec68b6ec9dae73046e9c34ac97439efda83/example/extensions/lib_subgraph/subgraph_lib.cc#L315

Node* input = graph->addNode(n->name + "_input", "null");

Mostly just little things like this. But this should be a lot of change for the better.

leezu commented 3 years ago

I think it would be ideal to use preprocessor #if to have bertpass_gpu.cc support both MX 1.7 and 1.8

MoisesHer commented 3 years ago

I think it would be ideal to use preprocessor #if to have bertpass_gpu.cc support both MX 1.7 and 1.8

thanks for checking this. I will make the modifications as suggested to support both MXNet 1.7 & 1.8