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

python API版の同梱ライブラリを完全なものにする #266

Open
qwerty2501 opened this issue Sep 10, 2022 · 13 comments
Open

python API版の同梱ライブラリを完全なものにする #266

qwerty2501 opened this issue Sep 10, 2022 · 13 comments

Comments

@qwerty2501
Copy link
Contributor

内容

現在PyO3版では以下の問題点が存在する

  • onnxruntimeライブラリが同梱されていない
  • cuda版、directml版で必要な追加ライブラリを手に入れやすくする手段が存在しない

これを解決するために以下の対応を行ったほうが良さそう

  • onnxruntimeのライブラリをwhlファイルに同梱する
  • cuda版、directml版で必要な追加ライブラリをダウンロードするsetup scriptを実装する

Pros 良くなる点

python API版voicevox_coreの使い勝手が向上する

Cons 悪くなる点

なし

実現方法

onnxruntimeについてはwhlファイルを作成する際に一緒に同梱させてあげるだけで良さそう
cuda版、directml版で必要な追加ライブラリについては setup script内で https://github.com/VOICEVOX/voicevox_additional_libraries からダウンロードするようにする

@Hiroshiba
Copy link
Member

wheelに同梱してしまうという手の他にも、setup.py実行時に動的にダウンロードするという手もあるかもと思いました。
(ただこっちの方法はwheelの場合でも可能なのか不明です)

どちらの方法も可能な場合、いったんどちらの方法でも良いと思いますが、ビルド時間の削減を考えるとどちらかといえば動的DLだと比較的嬉しいかもです。

@qwerty2501
Copy link
Contributor Author

wheelに同梱してしまうという手の他にも、setup.py実行時に動的にダウンロードするという手もあるかもと思いました。

onnxruntime動的にダウンロードするほうがめんどくないですか?
ビルド時間についてもそもそもcargo でビルドする際にonnxruntimeはすでにダウンロード済みであとはwheelに同梱する場所へコピーしてあげれば良さそうなのでそこまでビルド時間は伸びないはずです

@Hiroshiba
Copy link
Member

たしかに!どちらでも問題ないなと思いました!

@qwerty2501
Copy link
Contributor Author

優先度低に設定されてますが、次のバージョンでengineから使用するvoicevox_coreライブラリをpython api版にする場合はこの対応の優先度は上がりそうです。

@PickledChair
Copy link
Member

優先度低に設定されてますが、次のバージョンでengineから使用するvoicevox_coreライブラリをpython api版にする場合はこの対応の優先度は上がりそうです。

私も今日同じことを考えていました。python API 版の作業の進捗速度に応じて、 VOICEVOX/voicevox_engine#454 の作業(共有ライブラリ読み込みの仕組みを新しいコアに対応させる)に早めに取り組むべきかどうかが決まりそうだったので……(現在の開発版コアは API が大きく変わったので現在のエンジンから読み込むことができない)。

@Hiroshiba
Copy link
Member

実はエンジン側はもうライブラリDL機構があるので、このissueの解決は必須ではないんですよね。

ライブラリを同梱するかしないかでエンジン側のActionsのコードは変わりそうです。
が、新APIを使う周りのpythonコードやpip installする周りは変わらないという認識です。
なのでこのissueの解決有無がどちらであってもエンジン側からpython APIを使うことはできるから、優先度は高くなくても大問題にはならないのかなと思いました。

ただ、確かに同梱するなら先に同梱しておいたほうがエンジン側の対応が楽そうだと思います。
そういう意味だと優先度を上げても良さそうです。

wheelにライブラリを同梱することの難度によりそうだなと思いました。
@qryxip さんこのあたりご存知だったりしますか 👀

@qwerty2501
Copy link
Contributor Author

wheelよくわかってないんですが、同梱してなくても大丈夫なんですかね?
それであれば優先度低でも大丈夫そうですね。

@Hiroshiba
Copy link
Member

個人的には、onnxruntimeライブラリはコアを動かすために必須なので同梱に賛成ですが、CUDAとDirectMLに関してはsetup.pyでのダウンロードの難度が高いなら必須ではないかなと思っています。

CUDAとDirectMLはDownloaderがあるのでユーザーにとってあまり手間がかからないはずです。
一方で、需要も超多いというわけでもない気がしています(CPUは需要が多そう)。
手間がかかりそうだとわかったら、積極的に避けても良いのかなと思っています。

@Hiroshiba
Copy link
Member

wheelよくわかってないんですが、同梱してなくても大丈夫なんですかね? それであれば優先度低でも大丈夫そうですね。

ちょっと僕も自信微妙にないのですが、たぶんpip installしたときにライブラリディレクトリにdllを展開するだけなので、同梱しなくても動かすことはできるだろうなと思ってます。

@qwerty2501
Copy link
Contributor Author

ほなengineで問題が出たら優先度挙げる感じで 👍

@Hiroshiba
Copy link
Member

で挙げられているバージョン管理を念頭に置きつつ、エンジンからのPython API版の立ち位置を考え直してみました。
結論から言うと、Python API版を今すぐエンジンから使うのは結構難しく、将来的に移行するかも、という立ち位置になりそうです。

以下整理です。

  • エンジン側はコアのバージョン管理機構がある
    • コアはRust化のときにAPIが変わった
    • ので、エンジン側は2つのAPIに対応する必要がある
  • エンジン側のコアバージョン切り替え機能は動的ライブラリ用になっている
    • Python API用にバージョン切り替えするのは、↑とは別の実装がいる
    • 未調査だが、core.pydの切り替えになる
    • エンジン側はNuitka/Pyinstallerのビルドを挟んでおり、それでも実行時にcore.pydが切り替えられるのか現時点で不明。
  • コアバージョン管理を考えたとき、取れる選択肢は以下の4つがある(上ほどエンジンにとって楽)
    1. コアに下位互換性を持たせ、昔のAPIをDUPLICATEDで保持する
    2. エンジン側に2種類のコア動的ライブラリ切り替え機構を作る
    3. 最新のコアはPython APIを利用し、以前のコアは動的ライブラリ切り替え機構を使う
    4. 新しいコア用にPython API版ライブラリ切り替え機構を作り、以前のは今までのを使う
      全体として一番時間がかからないのは1番だと思います。
      あと、2,3を飛ばして1→4に移れるので嬉しさはありそうです。

ただ、コアはせっかく新しくなったから下位互換性は切り捨てても良さそうなのと、last_error_messageの実装を復活させる必要があったりするので、2も現実的な印象です。
この方法だと2→3という手もありそうです。

どちらの方法も最初は1か2が現実的で、時間的に余裕ができたり、機能的に必要になったら3,4の方法でpython API版に移行する、ということになりそうです。

(当初の予定とは違った形になってしまいました、申し訳ないです 🙇‍♂️ @qwerty2501

@qwerty2501
Copy link
Contributor Author

cbindgen で出力制御できるようです https://github.com/eqrion/cbindgen/blob/master/docs.md#writing-your-c-api
単純にpub外せば良さそう?

@qwerty2501
Copy link
Contributor Author

いやpubつけないとそもそも dynamic libraryに出力されないか

こっちがただしそう https://github.com/eqrion/cbindgen/blob/master/docs.md#ignore-annotation

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

No branches or pull requests

3 participants