Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of export config parameters #113

Open
espnetUser opened this issue May 29, 2024 · 2 comments
Open

Incorrect handling of export config parameters #113

espnetUser opened this issue May 29, 2024 · 2 comments

Comments

@espnetUser
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

Would you create a PR for this?

Sure. #116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants