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

Onnxrt execution provider configs #992

Merged
merged 34 commits into from
Jul 5, 2024

Conversation

manickavela29
Copy link
Contributor

@manickavela29 manickavela29 commented Jun 12, 2024

Config files handling for onnxrt provider
cc : #40

@manickavela29 manickavela29 marked this pull request as draft June 12, 2024 02:05
@manickavela29
Copy link
Contributor Author

manickavela29 commented Jun 24, 2024

Hi,
I haven't tried with any Offlinemodel, so I am extending the config support to only for OnlineModel,
but easily it can be extended for OfflineModel.
Let me know if you have any better suggestions for maintaining the config.

session.cc files also has several other models (vad,etc), since these are minimal models, they are left as they are
if the support for trt/cuda backends can be extended for them.
let me know

for pybind interface, still adding in changes

@manickavela29 manickavela29 marked this pull request as ready for review June 24, 2024 17:47
@manickavela29 manickavela29 marked this pull request as draft June 24, 2024 18:29
@manickavela29 manickavela29 marked this pull request as ready for review June 25, 2024 15:41
sherpa-onnx/csrc/online-model-config.cc Outdated Show resolved Hide resolved
sherpa-onnx/csrc/online-model-config.h Show resolved Hide resolved
sherpa-onnx/csrc/online-recognizer.cc Outdated Show resolved Hide resolved
sherpa-onnx/csrc/provider-config.cc Outdated Show resolved Hide resolved
sherpa-onnx/csrc/provider-config.h Outdated Show resolved Hide resolved
sherpa-onnx/csrc/session.cc Outdated Show resolved Hide resolved
sherpa-onnx/csrc/session.cc Outdated Show resolved Hide resolved
@csukuangfj
Copy link
Collaborator

Hi, I haven't tried with any Offlinemodel, so I am extending the config support to only for OnlineModel, but easily it can be extended for OfflineModel. Let me know if you have any better suggestions for maintaining the config.

session.cc files also has several other models (vad,etc), since these are minimal models, they are left as they are if the support for trt/cuda backends can be extended for them. let me know

for pybind interface, still adding in changes

Thanks! That sounds good to me.

@csukuangfj
Copy link
Collaborator

Please have a look at errors reported by clang-tidy
https://github.com/k2-fsa/sherpa-onnx/actions/runs/9665507110/job/26662771827

and the style check by cpplint.

https://github.com/k2-fsa/sherpa-onnx/actions/runs/9665507136

(Note: You can run these two tools locally.)

@manickavela29
Copy link
Contributor Author

I wanted to keep provider config unique for each models, this would give good control for CUDA and TRT configs,
But if developers give different devices(outside of cuda/trt) is a complex scenario and it must be handled, so I remove those patches midway.

Most of the fixes I had also went away with those patches thats why several bugs

@manickavela29
Copy link
Contributor Author

Please have a look at errors reported by clang-tidy https://github.com/k2-fsa/sherpa-onnx/actions/runs/9665507110/job/26662771827

and the style check by cpplint.

https://github.com/k2-fsa/sherpa-onnx/actions/runs/9665507136

(Note: You can run these two tools locally.)

yes, have been using for cpplint, had a hard time with exit 🙂
will try with clangtiddy

@csukuangfj
Copy link
Collaborator

By the way, you can use

pip install clang-tidy

@manickavela29 manickavela29 requested a review from csukuangfj June 26, 2024 05:46
@manickavela29
Copy link
Contributor Author

manickavela29 commented Jun 28, 2024

I ran the python tests in debug mode and got this error

  • python3 ./python-api-examples/offline-telespeech-ctc-decode-files.py
    Traceback (most recent call last):
    File "/mnt/efs/manickavela/asr_work/trt/sherpa-onnx/./python-api-examples/offline-telespeech-ctc-decode-files.py", line 16, in
    import sherpa_onnx
    File "/mnt/efs/manickavela/miniconda3/envs/icefallmk/lib/python3.9/site-packages/sherpa_onnx/init.py", line 1, in
    from _sherpa_onnx import (
    ImportError: arg(): could not convert default argument 'trt_config: sherpa_onnx::TensorrtConfig' in method '<class '_sherpa_onnx.ProviderConfig'>.init' in
    to a Python object (type not registered yet?)

it would be great to have some hints for fixing this
I tried to take reference from vad /online model config implementation and attempted but it didn't help.
I will dump a patch with my experiments and later clear it

@manickavela29
Copy link
Contributor Author

Hi @csukuangfj, would be great to have some leads on python fix

sherpa-onnx/python/csrc/sherpa-onnx.cc Outdated Show resolved Hide resolved
sherpa-onnx/python/csrc/provider-config.cc Outdated Show resolved Hide resolved
@csukuangfj
Copy link
Collaborator

please also change
https://github.com/k2-fsa/sherpa-onnx/blob/master/sherpa-onnx/python/sherpa_onnx/online_recognizer.py#L174

I give you an example below. You need to change other def from_xxx() accordingly.

Change

model_config = OnlineModelConfig(
transducer=transducer_config,
tokens=tokens,
num_threads=num_threads,
provider=provider,
model_type=model_type,
modeling_unit=modeling_unit,
bpe_vocab=bpe_vocab,
debug=debug,
)

to

        provider_config = ProviderConfig(provider=provider)

        model_config = OnlineModelConfig(
            transducer=transducer_config,
            tokens=tokens,
            num_threads=num_threads,
            provider_config=provider_config,
            model_type=model_type,
            modeling_unit=modeling_unit,
            bpe_vocab=bpe_vocab,
            debug=debug,
        )

Remember to change

 ProviderConfig,
 OnlineModelConfig, 

sherpa-onnx/python/csrc/tensorrt-config.cc Outdated Show resolved Hide resolved
sherpa-onnx/python/sherpa_onnx/online_recognizer.py Outdated Show resolved Hide resolved
@manickavela29 manickavela29 requested a review from csukuangfj July 4, 2024 06:16
@manickavela29 manickavela29 requested a review from csukuangfj July 4, 2024 07:03
@manickavela29
Copy link
Contributor Author

Hi @csukuangfj,
would be great if you can have one more review by today. 🙂

Copy link
Collaborator

@csukuangfj csukuangfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Left some minor comments. Otherwise, it looks good to me.

sherpa-onnx/csrc/provider-config.cc Show resolved Hide resolved
sherpa-onnx/python/csrc/sherpa-onnx.h Outdated Show resolved Hide resolved
sherpa-onnx/python/sherpa_onnx/__init__.py Outdated Show resolved Hide resolved
@manickavela29 manickavela29 requested a review from csukuangfj July 5, 2024 05:39
@manickavela29
Copy link
Contributor Author

Hi @csukuangfj,

I think it is all good now, expected tests are also passing through,
let me know if there are any more reviews

@manickavela29 manickavela29 changed the title Onnxrt ep config Onnxrt execution provider configs Jul 5, 2024
Copy link
Collaborator

@csukuangfj csukuangfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@csukuangfj csukuangfj merged commit 55decb7 into k2-fsa:master Jul 5, 2024
147 of 184 checks passed
@manickavela29
Copy link
Contributor Author

uffff, Thank you so much

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

Successfully merging this pull request may close these issues.

3 participants