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

add: VOICEVOX CORE用の初期化経路を構築する #6

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jun 25, 2024

内容

VOICEVOX/voicevox_core#802 用に、

  1. バージョン文字列("1.17.3")を定数として取得できるようにします。
  2. __init-for-voicevox featureを追加し、VOICEVOX CORE用にort本来のものとは異なる初期化経路を使えるようにします。この初期化経路は結果をResultで返し、失敗しても別のlibonnxruntimeに対してやりなおすことができます。

関連 Issue

ref VOICEVOX/voicevox_core#802

スクリーンショット・動画など

その他

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

(よくわかってないですが)ほぼ lgtm です!!

とりあえずメモとして残しておきたいのですが、これってコア側でonnxruntimeの動的読み込みを可能にするための機能なんでしたっけ。
それとも動的読み込みとビルド時リンクを切り替えられるようにするためとか・・・?

ort-sys/build.rs Outdated Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jun 29, 2024

とりあえずメモとして残しておきたいのですが、これってコア側でonnxruntimeの動的読み込みを可能にするための機能なんでしたっけ。 それとも動的読み込みとビルド時リンクを切り替えられるようにするためとか・・・?

ortは元々両方可能ですね。バージョン文字列の埋め込みを除いた目的は次の二つなのですが、これらだけのために200行超の実装を重ねるべきか今はちょっと迷っています。

  1. 初期化の失敗をパニックじゃなくてResultにするため

    ortの初期化は失敗したときにResult::Errを返すのではなくパニックします。例えばVOICEVOX ENGINEのようなアプリケーションから利用されているときはPythonインタプリタごとクラッシュします。
    (パニックを"catch"することはできるのですが、メッセージを文字列として得ることはできません。ユーザーにはパニックハンドラがstderrに吐き出したメッセージを読んでもらうことになります。パニックハンドラからは中身が見れるのでそこを細工すればエラーとして持って来れるかもしれませんが、パニックハンドラはグローバルなのでRust APIとしてはそこを弄るのは避けたいです。Rust API以外だったらパニックハンドラを弄ってよいかもしれませんが、弄っているうちに結局200行超嵩みそうな気がしています)

    thread 'main' panicked at {ソースコード位置}:
    {voicevox-ortのエラーメッセージ}
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

    あとは間違えたlibonnxruntime (e.g. C:\Windows\System32\onnxruntime.dll)を一度選んでしまったら以後どうにもできずフォールバックとかができないことですが、今よく考えるとlibonnxruntimeのフォールバックをしたいユースケースというのはあまり無さそうかなと思っています。なのでパニックにしたときの問題は上記で述べた、パニックをユーザーにどう見せるかになってくると思います。

  2. 「初期化済みシングルトンインスタンス」をvoicevox-ort側で持っておくことにより、voicevox_coreとsharevox_coreを併用したときの利用をスムーズにするため

    こっちはRust API限定ですし、まあおまけとして。

@qryxip qryxip marked this pull request as draft June 29, 2024 12:59
@qryxip
Copy link
Member Author

qryxip commented Jun 29, 2024

  • bbf69cd (#6) エラー(と警告)のメッセージを変更しました。

@qryxip qryxip marked this pull request as ready for review June 29, 2024 17:13
@Hiroshiba
Copy link
Member

なるほどです、ありがとうございます!

パニックにならないのは良いことだと思います!
本家に入っても良いかもしれない。

src/lib.rs Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jun 30, 2024

パニックにならないのは良いことだと思います! 本家に入っても良いかもしれない。

本家に入るのならfeature名も__init-for-voicevoxじゃなくて__require-explicit-initとかにできるかもですね。

ただpykeio/ortはONNX Runtimeの静的リンクに舵を切ってるので実装量増加+依存ライブラリ一つ(once_cell)の追加が許容されるかどうか… と思いましたが、CUDA版とかは静的リンクはまだまだ無理ですしまあissue/PR出してもいいかもしれない。

@qryxip qryxip requested a review from Hiroshiba June 30, 2024 08:17
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

@qryxip qryxip merged commit 07c047c into VOICEVOX:main Jul 1, 2024
7 checks passed
@qryxip qryxip deleted the add/init-for-voicevox branch July 1, 2024 19:33
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.

2 participants