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

change: Onnxruntime型を追加し、そこからdlopen/LoadLibrary*を行う #802

Merged
merged 26 commits into from
Jul 4, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jun 25, 2024

内容

パブリックAPIにOnnxruntime型を追加し、そこからlibonnxruntimeのdlopen/LoadLibrary*をするようにします。

Onnxruntime

Synthesizerのコンストラクトはこのようになります。

ort = Onnxruntime.load_once()
ojt = OpenJtalk("./open_jtalk_dic_utf_8-1.11")
synth = Synthesizer(ort, ojt)

オプションとしてDLLの"filename"を指定することもできます。

ort = Onnxruntime.load_once(filename="./path/to/onnxruntime.dll")

あと、supported_deviceOnnxruntimeのメソッドになりました。

supported_devices = ort.supported_devices()

iOSのXCFramework

Rust APIとC APIだけ、dlopen/LoadLibrary*ではなくロード時動的リンクで動く形でビルドできるようにしています。

C APIだと、GHAでのパッケージング時に次のどちらかのコメントアウトをsedで外すようにします。

//#define VOICEVOX_ONNXRUNTIME_LIBLOADING
//#define VOICEVOX_ONNXRUNTIME_LINK_DYLIB

#if !(defined(VOICEVOX_ONNXRUNTIME_LIBLOADING) || defined(VOICEVOX_ONNXRUNTIME_LINK_DYLIB))
#error "either `VOICEVOX_ONNXRUNTIME_LIBLOADING` or `VOICEVOX_ONNXRUNTIME_LINK_DYLIB` must be enabled"
#endif

#if defined(VOICEVOX_ONNXRUNTIME_LIBLOADING) && defined(VOICEVOX_ONNXRUNTIME_LINK_DYLIB)
#error "`VOICEVOX_ONNXRUNTIME_LIBLOADING` or `VOICEVOX_ONNXRUNTIME_LINK_DYLIB` cannot be enabled at the same time"
#endif
  • VOICEVOX_ONNXRUNTIME_LIBLOADINGの場合、dlopen/LoadLibrary*でlibonnxruntimeをロードします。
  • VOICEVOX_ONNXRUNTIME_LINK_DYLIBの場合、libonnxruntimeをロード時動的リンクします。load_onceinit_onceとなり、filenameのオプションが消失します。

iOSのXCFrameworkのみVOICEVOX_ONNXRUNTIME_LINK_DYLIBで、他はVOICEVOX_ONNXRUNTIME_LIBLOADINGです。

libonnxruntimeとlibvoicevox_onnxruntime

Onnxruntime.LIB_NAME = "onnxruntime"としていますが、VOICEVOX/voicevox_project#24以降はこれを"voicevox_onnxruntime"にする方向で考えています。考えているのは大体次の通りです。

  1. libonnxruntimeも普通に読めるようにした上で、リポジトリ上のテストでは引き続きlibonnxruntimeを使う
  2. VOICEVOX_ONNXRUNTIME_LINK_DYLIBの場合リンク対象をlibonnxruntimeのままにする。変えたければpatchelfinstall_name_toolでバイナリを変更するという形 (実際、今のCOREのXCFrameworkをビルドするにあたってはinstall_name_toolが使われています)
  3. voicevox-ortはlibvoicevox_onnxruntimeのダウンロードを行わない。ユーザーはダウンローダーでlibvoicevox_onnxruntimeをダウンロードする

関連 Issue

Resolves #721.
Fixes #537.
Closes #445.

その他

Cargo.toml Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jun 25, 2024

まだドキュメントやテストが詰めきれていませんが、とりあえずこのパブリックAPIがUX的にどうなのかご意見を頂けたらなと思っています。

@qryxip
Copy link
Member Author

qryxip commented Jun 27, 2024

onnxruntime-libloadingonnxruntime-link-dylibですが、実行時挙動はどうにもならないとして、APIの形だけでも一つにまとまらないかと考えてみました。macOSのXCFrameworkや、Swift API作成のことを考えるとAPIの形が分かれているのが気になった次第です。

以下のようにすれば、onnxruntime-link-dylibonnxruntime-libloadingから単にできることを減らしたものになるはずなので、ドキュメントをちゃんと書けばユーザーを驚かせずにすむのではないかと思いました。
(着想を得たのはPythonのctypesです。どういう理由で用意されているかはわかりませんが、Unix限定でctypes.CDLL(None)というのができます)

class Onnxruntime:
    LIB_VERSIONED_FILENAME: str

    # Python APIやJava APIにおいては`filename=None`は実際には許可しない
    def init_once(filename: str | None = LIB_VERSIONED_FILENAME) -> Self:
        ...
  • Windows, libloading, filename=Some("…"): LoadLibraryExW(_, filename, _)
  • Windows, libloading, filename=None: 「Windowsでは無理」というエラー
  • Windows, link-dylib, filename=Some("…"): GetModuleHandleExW(_, filename, _)を開き、そこからのOrtGetApiBase()の戻り値がロード時動的リンクされている方のものと一致しているか検査。違っていたらエラー。検査後はロード時動的リンクされているAPI達を使っていく
  • Windows, link-dylib, filename=None: 「Windowsでは無理」というエラー
  • Unix, libloading, filename=Some("…"): dlopen(filename, _)
  • Unix, libloading, filename=None: dlopen(NULL, _)
  • Unix, link-dylib, filename=Some("…"): 「dlopenは禁止されている」というエラー
  • Unix, link-dylib, filename=None: dlopenはせずにリンクされたONNX Runtimeの関数を利用する (dlopen(NULL, _)の方も似た挙動になるはず)

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 27, 2024

@qryxip とりあえず最後のコメントへのコメントです!

ctypes.CDLLのような形、面白いですね。UXとしてありな気がしました!

Windows, link-dylib, filename=None: 「Windowsでは無理」というエラー

このときって「ロード時動的リンクされている方のもの」は使えない感じですかね?
たぶん今のコアがこの形(ビルド時リンク)だと思ってて、だとしたらできるんじゃないかなと思った次第です。
というのも、たぶんエンジンもこの形になるので、雑にNULLでいけるなら楽そうだな〜と。

他は僕の知識量からコメントできることはなさそうでした!!
UX的には結構便利そうに感じます。

追記:あ、この辺りはそもそもWindowsで2種類作るのかによりそう。
1種のがよければエンジン側が頑張って合わせるのが良さそう。


あ。仮称かもなのですが、libloadinglink-dylibの名称がややこしめなのがちょっと気になりました!
たぶんdylibは避けて、役目で分けちゃうのがわかりやすいかなと。
で、「iOSのdylibはこれ使うと良いよ」みたいな案内がある感じとか。

名称・・・この辺りのリンク方法?、なんて呼ぶんですかね。。。
libloadingは実行時の読み込みだから、dynamic-loadとか・・・?
link-dylibはビルド時のリンクだから、buildtime-linkとか・・・?
うーん。。。

2値だから1つにまとめてtrue/falseで制御するとかもありかも。
だとしたらlinkする(link-dylib)かしない(libloading)かだから、onnxruntime-linktruefalseとか・・・・・?

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.

一通り全部眺めました!!良さそう!!!

サンプル見た感じとてもわかりやすいですね!!
特にPythonとかの。

C APIのサンプルは、たぶん一方のしかないからわかりやすいんだろなーと思いました。
ビルド時リンクか動的読み込みかにかかわらず、同じ関数を使えるようになれば、実際に使うときも混乱を減らせる気がします・・・!!

いや〜〜〜楽しみですね!!!!!

example/kotlin/README.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jun 28, 2024

Windows, link-dylib, filename=None: 「Windowsでは無理」というエラー

このときって「ロード時動的リンクされている方のもの」は使えない感じですかね? たぶん今のコアがこの形(ビルド時リンク)だと思ってて、だとしたらできるんじゃないかなと思った次第です。

SymInitialize(W)を使ってよいのなら「externなONNX RuntimeのAPIに対する実体を持っているonnxruntime.dll」を見つけだすことができるのですが、ちょっとこれを使うのには抵抗を覚えました。

というのも、たぶんエンジンもこの形になるので、雑にNULLでいけるなら楽そうだな〜と。

追記:あ、この辺りはそもそもWindowsで2種類作るのかによりそう。 1種のがよければエンジン側が頑張って合わせるのが良さそう。

単に"onnxruntime.dll"とだけ指定すれば前もってctypes.CDLLで開いた方のonnxruntime.dllを開いてくれるので、compatible_engineだとそれでいいのではと思います。voicevox_onnxruntime.dllでも同様 (確かめました)。

あ。仮称かもなのですが、libloadinglink-dylibの名称がややこしめなのがちょっと気になりました! たぶんdylibは避けて、役目で分けちゃうのがわかりやすいかなと。 で、「iOSのdylibはこれ使うと良いよ」みたいな案内がある感じとか。

2値だから1つにまとめてtrue/falseで制御するとかもありかも。 だとしたらlinkする(link-dylib)かしない(libloading)かだから、onnxruntime-linktruefalseとか・・・・・?

RustのfeatureはAPIの拡張を行うもので「デフォルトの挙動」を変えるべきではないとされているので、それでやるとしたらonnxruntime-link | onnxruntime-no-linkの二択で両方選択したらコンパイルエラーという形ですね。
(といってもfeatureでデフォルトの挙動を変えるクレートは世に溢れていて、ortもその一つだったりするのですが…)

名称・・・この辺りのリンク方法?、なんて呼ぶんですかね。。。 libloadingは実行時の読み込みだから、dynamic-loadとか・・・? link-dylibはビルド時のリンクだから、buildtime-linkとか・・・? うーん。。。

「静的リンク」, 「ロード時動的リンク」, 「実行時動的リンク」の三つのどれかなのか混乱しないような名前ですかね…
(pykeio/ortはONNX Runtimeの静的リンクをやっているライブラリだったりするし)

@Hiroshiba
Copy link
Member

SymInitialize(W)を使ってよいのなら「externなONNX RuntimeのAPIに対する実体を持っているonnxruntime.dll」を見つけだすことができるのですが、ちょっとこれを使うのには抵抗を覚えました。
単に"onnxruntime.dll"とだけ指定すれば前もってctypes.CDLLで開いた方のonnxruntime.dllを開いてくれるので、compatible_engineだとそれでいいのではと思います。voicevox_onnxruntime.dllでも同様 (確かめました)。

よくわかってないのですが、エンジン側はたしかに名前指定で良さそうに思いました!
よくよく考えれば今そんな感じになってました。ファイル名で検索して見つかったらそれ指定、みたいな感じだった記憶。

となると、掲げられた2^3パターンの挙動で良さそう感!!

RustのfeatureはAPIの拡張を行うもので「デフォルトの挙動」を変えるべきではないとされているので

おおーなるほどです。それはそうかも。
となると、片方をデフォルトにし、もう片方はプラスにするみたいなのもありかも?
今のmainブランチの実装(ビルド時にリンクする)をデフォルトとするならonnxruntime-no-linkだけ、ビルド時リンクしないのをデフォルトにするならonnxruntime-linkで良さそう。link-on-buildとかでも良いかもしれない。

「静的リンク」, 「ロード時動的リンク」, 「実行時動的リンク」の三つのどれかなのか混乱しないような名前ですかね…

link-on-buildlink-on-runtimeとかで良い気もしてきました。
まあ最悪注釈コメントかドキュメントがあれば別に良いかも。

@qryxip
Copy link
Member Author

qryxip commented Jun 28, 2024

onnxruntime-loadonnxruntime-link-cdylibでどうでしょうか?
「ロード時動的リンク」(run-time dynamic linking)ってあまり言わない気がしますし、"link"で通じそうな気がします。
(といっても正確に区別するときにロード時動的リンクと言ったりはするのですが)

追記: 静的リンクをやる予定が一切無いならonnxruntime-linkでもいいかも?

@Hiroshiba
Copy link
Member

onnxruntime-loadとonnxruntime-link-cdylibでどうでしょうか?
「ロード時動的リンク」(run-time dynamic linking)ってあまり言わない気がしますし、"link"で通じそうな気がします。

良さそう!!
onnxruntime-linkも良さそう。他にはonnxruntime-load-cdylibも良さそう!
どれもややこしさは解消されてると感じます!!

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

qryxip commented Jun 29, 2024

b1dca9c (#802)
f0c720c (#802)
4af36e0 (#802)
249f8f3 (#802)
7b57268 (#802)
e8dc442 (#802)
フィーチャ名はload-onnxruntimelink-onnxruntimeにしてみました。

…ちょっと"load"と"link"で目が滑りそう。load-ortlink-ortにしたらましになるかも?
(実際今CIが落ちまくったのは、私が"load"と"link"を二箇所で間違えたためです)

@qryxip
Copy link
Member Author

qryxip commented Jun 29, 2024

#802 (comment)
放送でも話したのですが、やはりどうあがいても二つは別々の挙動ですし、Swift APIも多分XCFramework経由でONNX Runtimeを読むことになりそう(i.e. "load"ではなく"link"で固定)なのでこのアイデアは取り止める方向にしようと思います。

@qryxip
Copy link
Member Author

qryxip commented Jun 29, 2024

  • c18e335 (#802): build_and_deploysedをし損ねていたので直しました。
  • 1492995 (#802): ドキュメントの表記のずれとかを直しました。
  • e619d8b (#802): C APIにOnnxruntime::LIB_{,UN}VERSIONED_FILENAMEのgetterを追加しました。

@qryxip
Copy link
Member Author

qryxip commented Jun 30, 2024

c79c3f2 (#802)
init_onceload_onceの片方のみが利用であることがC APIのドキュメントからわからないので、"Availability"というセクションを追加してみました。

image
image

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 30, 2024

バイナリのリリースではiOSだけlinkで、他は全部loadでいいと思います (今のPRの状態がそうなっています)。

こちらは賛成です!

あと気づいたのですが、C 言語・Rust以外もlink/load片方だけ提供で良さそうなことに気づきました!
なのでCとRustのドキュメントがOK なら 良さそう!

Availability

良さそう! だけど結構見ない人多そう・・・! それでもここに案内があればそれで良さそう!


UX に関して色々考えてみました!
結論から言うと、関数は分けていいと思います!!

先ほどの通り、CとRust以外のドキュメントはそもそもlink/load片方しか出ないので無視できるはず。
あとはCとRustでどう調理するかかなと!

RustとC言語APIは、結構プログラミングを知ってる人を想定して良いと思います。
なので、結構プログラミングが分かってる人に伝わるドキュメントが書けるならOK。

Rustユーザーは相当分かっていると思うので、link/loadをそのまま 説明してもOK。
link用とload用の関数が分かれているのもOK。多分むしろ別れていた方が良い。

Cユーザーはわりとライト層だと思うので、link/loadをそのまま説明するのは避けた方が良さそう。
代わりに「iOSかどうか」で説明を分けるのなら伝わりそう。この説明だったら別れてても良いはず。

Rust向けドキュメントは置いといて、C向けドキュメントは各関数ごとに「iOSでしか使えないか、iOS以外でしか使えないか」が案内されてれば良さそう!
一応ちゃんとわかってる人向けに、リンクなのかロードなのかみたいな説明もあっても良さそう。

Cユーザー向けのVOICEVOXコアの使い方がいろんな記事で紹介され始めたらもっと簡略化しても良さそう。
それまではちょっとメンテナンスめんどくさいけど丁寧かつ初学者にもちょっと分かりやすい説明の方が良さそう

Rust向けドキュメントは・・・お任せします!!
多分 @qryxip さんが分かりやすい形が一番わかりやすい!

@qryxip
Copy link
Member Author

qryxip commented Jul 1, 2024

  • dc587ae (#802): C APIでの紹介を「iOSか否か」にしてみました。またRust APIのCargo feature部分に書いていたコメントを、Rust APIのrustdocに移した上で説明も詳細化しました。

    image
    image

  • e99b293 (#802): C APIのビルドについて書いてあるドキュメントにヘッダの書き換えについて書いてなかったので、付け加えました。

@qryxip qryxip marked this pull request as ready for review July 1, 2024 19:34
@qryxip
Copy link
Member Author

qryxip commented Jul 2, 2024

c545f89 (#802)
f60cd0e (#802)
このPRの主旨から若干外れますが、cargo test -p voicevox_core --features {load,link}-onnxruntime (非--workspace)がdoctestを含めて通るようにしました。
(このPRで織り込んだdoctest用の工夫が今後壊れるかもしれないため)
追記: CIはそのままにしました。非--workspaceビルドでのテストはそんなに重要じゃないので。

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!!

ドキュメントはもしかしたらリリースまでに変更させていただくかもですが、とりあえずマージして進めるのは問題なさそう!!
楽しみですね!!

もしかしたらまだ何か残ってるかもなのでマージせず置いています。
問題なければマージしていただければ!!

.github/workflows/generate_document.yml Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jul 2, 2024

@Hiroshiba
Copy link
Member

ちょんと把握できてないのですが、とりあえず問題なければ(mainにマージすればテストは通るとか、リリースを一度すれば通るとか)マージしても大丈夫かなと!

@qryxip
Copy link
Member Author

qryxip commented Jul 3, 2024

問題はある(ダウンローダーで--device directmlを指定するとonnxruntime_providers_cuda.dllまで付いてくるし、リリースはできてもdownload_testは落ちる)のですが、onnxruntime-rsからortに切り替えた時点でこの問題は発生しており、このPRに起因はしていないと思います。

@qryxip
Copy link
Member Author

qryxip commented Jul 3, 2024

#804

@Hiroshiba
Copy link
Member

すいませんちょっとわかっておらず。。。 🙇
あ! ortに切り替える時点でdownload_testは落ちるはずだったんだけど、download_testの稼働条件にcargo.tomlの更新が含まれいなくてテストが回ってなく、このプルリクエストで初めてテストが回った、みたいな感じでしょうか?

ま~だとしたらこのプルリクエストはマージしちゃってもいいのかなと思いました!!
ただmainブランチのテストが落ちちゃうことにはなる?ので、気になるようでしたら先にそっちを修正でもいいかなと。

どちらでも!!別にいいならマージしていただければ!
(個人的にはどっちにしろ一長一短ですが、このプルリクエストは大きいのでとりあえずマージが進めやすいのかなと思いました。)

@qryxip
Copy link
Member Author

qryxip commented Jul 3, 2024

mainブランチのテストが落ちちゃうことにはなる?ので

このリポジトリでdownload_testが最後に動いたのは5ヶ月前ですね。
https://github.com/VOICEVOX/voicevox_core/actions/workflows/download_test.yml

download_testの稼働条件にcargo.tomlの更新が含まれいなくてテストが回ってなく、このプルリクエストで初めてテストが回った、みたいな感じでしょうか?

こんな感じです…

# 新しいビルドができるまで、このworkflowが壊れて動かないことを許容する
# https://github.com/VOICEVOX/voicevox_core/issues/741#issuecomment-1935303742
#push:
# branches:
# - main
#pull_request:
# paths:
# - "Cargo.*"
# - "crates/downloader/**"
# - ".github/workflows/download_test.yml"

私が貼ったGHAのログは、qryxip/voicevox_coreで「バージョン999.999.999のリリース」を行うことによってdownload_testを作動させたものです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 4, 2024

あーーーーーーなるほどです!!!
このPRのテストが落ちていたのが、download_test由来だと勘違いしてました!!!
そうか、テスト落ちるよねってことでそういえば停止してましたね。。。

追加コミットも見ました、コード的にはOKなのでマージしてOKならしていただければ!!
(完了のタイミングがわからない)

@qryxip qryxip merged commit 537d9b2 into VOICEVOX:main Jul 4, 2024
38 checks passed
@qryxip
Copy link
Member Author

qryxip commented Jul 5, 2024

@takejohn 共有です!

  • Onnxruntimeが追加され、Synthesizerのコンストラクタに要求されるようになりました。
    • Python APIとJava APIでは、ラッパー言語側で「シングルトンインスタンス」を持つようにしています。
  • supported_devicesOnnxruntimeのメソッドになりました。
  • SynthesizerOnnxruntimeのgetterを持つようになります。
  • featureとしてload-onnxruntimelink-onnxruntimeが追加されました。ただしPythonとJavaではload-onnxruntimeで固定です。Node.jsもそうすることになると思います。

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