espnet / espnet_onnx

Onnx wrapper for espnet infrernce model
MIT License
152 stars 24 forks source link

Incorrect handling of export config parameters #113

Open espnetUser opened 4 months ago

espnetUser commented 4 months ago

Hi,

I noticed an issue when converting a Transducer model from espnet to espnet_onnx where the max_seq_len in the DefaultEncoder class was not properly set when specifying m.set_export_config(max_seq_len=5000) in top-level export script.

To me it looks like the issue is caused by incorrectly passing the export_config to the replace_modules method in get_encoder method here:

$ git diff espnet_onnx/export/asr/models/__init__.py
diff --git a/espnet_onnx/export/asr/models/__init__.py b/espnet_onnx/export/asr/models/__init__.py
index d40de3f..7db3bbb 100644
--- a/espnet_onnx/export/asr/models/__init__.py
+++ b/espnet_onnx/export/asr/models/__init__.py
@@ -56,7 +56,7 @@ def get_encoder(model, frontend, preencoder, export_config, convert_map):
             ),
             model,
             preencoder=preencoder,
-            export_config=export_config,
+            **export_config,
         )
         return DefaultEncoder(_model, frontend, **export_config)

For instance, with current code OnnxRelPositionalEncoding was always initialized with default value of 512 instead of what was specified in export_config dict.

@Masao-Someki: Could you please take a look and let me know if that change looks correct?

Thanks!

Masao-Someki commented 3 months ago

@espnetUser Sorry for the late reply! And thank you for reporting the bug!! It looks like your change does fix the issue! It seems the code passes the export_config to the class as a dictionary rather than as individual arguments...

Would you create a PR for this?

espnetUser commented 3 months ago

@Masao-Someki: Thanks for getting back to me!

Would you create a PR for this?

Sure. https://github.com/espnet/espnet_onnx/pull/116