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

動的ライブラリにモデルファイルを同梱する #99

Merged
merged 25 commits into from
Mar 23, 2022

Conversation

Oyaki122
Copy link
Member

内容

#86 を実装します
https://github.com/magcks/embed を改変したcmakeスクリプトを用いて、Windowsでは.rcファイル、Mac, Linuxでは.incbin命令を用いてバイナリを埋め込む仕組みです

関連 Issue

ref #86

その他

Windows11及びWSL2 Ubuntu20.04での動作を確認しています

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.

@Yosshi999 さん、 @PickledChair さん、もしよかったらレビュー頂けるととても助かります・・・!

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

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

ほぼ良さそうでした!(Mac でもビルドして動作確認してみましたが動きました。) 一部気になった部分にコメントしました。

また、コード外で他に2つ気になったことがあります:

  • open_jtalk ディレクトリが追加されていますが、混入でしょうか?(main ブランチはまだ open_jtalk に依存していなかった気がしました)
  • PR のタイトルが「静的ライブラリ」になっていますが、ビルド結果は dll なので「動的ライブラリ」が正しい?(タイトルは後でアップデート情報に記載されると思うので質問してみました)

core/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Yosshi999 Yosshi999 left a comment

Choose a reason for hiding this comment

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

一件コメントしましたが、変更しなくてもかまいません!

core/src/embedBin/FindEmbed.cmake Outdated Show resolved Hide resolved
@Oyaki122 Oyaki122 changed the title 静的ライブラリにモデルファイルを同梱する 動的ライブラリにモデルファイルを同梱する Mar 22, 2022
@Oyaki122
Copy link
Member Author

レビューありがとうございます!

静的ライブラリ

お恥ずかしい限りです… 修正しました

openjtalk

混入でしたので修正しました

その他の修正については各コメントにリプライしました

Copy link
Contributor

@Yosshi999 Yosshi999 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

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

LGTM! example/python/run.py の引数が変更された(--root_dir_path がなくなった)ので、この機会に README に反映すると良いかもしれません(「サンプル実行」と「コアライブラリのビルド」の項目に run.py の実行方法の記載があります)。

メモ

今回の変更に際して考慮すべきことがありそうだったのでメモします(勘違い等ありましたらご指摘ください)。

今回の変更で、全てのOSプラットフォーム向けの共有ライブラリが1つのリソース(各 onnx ファイルと metas.json)を共有していた状態から、各共有ライブラリごとに個別にリソースが埋まっている状態に変わります。この結果、GitHub Releases で配布する core.zip のサイズが大きくなるはずです。共有ライブラリは 10 種類のプラットフォーム向けにビルドしていて、OSS 版コアのリソースは 60 MB 弱なので、OSS 版の場合はおよそ 600 MB で済みます。一方で製品版コアのリソースは合計で 200 MB 超あると思うので、core.zip のサイズは単純計算で 2 GB 超となり、GitHub Releases のファイルサイズの制限を超える可能性があります。問題ない可能性もありますが……。

考えられる対処方法:

  • ZIP より圧縮率の高い圧縮方法を試す
    • 手元で 7-zip で圧縮してみた限りでは ZIP 版と大してサイズが変わらなかったので有効ではないかも
  • onnxruntime の配布方法のように、各プラットフォーム向けのバイナリごとに別々にアップロードする
    • libcore.dll, libcore.so, libcore.dylib はリネームする必要がなくなる。ただしエンジン側での読み込み方法の変更が必要となるため、互換性維持のためにリネームを続ける、という考えもありかもしれない

@y-chan
Copy link
Member

y-chan commented Mar 23, 2022

確かに、埋め込みによって色々変わりそうですし、intializeのインターフェース変更のため、エンジン側の対応(特にコアの複数バージョン対応あたり)が大変になりそうな予感です。

このPRはcpp-libraryブランチとコンフリクトしそうなので、既に発生しているmainとコンフリクトを解消する時に同時にコンフリクトを解消しようかなと思っていたのですが、一旦マージは後(エンジン等の対応がある程度できそうな目処が立ってから)にすべきかなと思いました。

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 23, 2022

たしかに2GB超えそうですね・・・。

エンジン側の対応が必要な件は、製品版coreのプレリリースがあったほうがやりやすそうに感じました。
実際にマージしちゃったほうがビルドしやすいので、一旦マージできるとうれしいかも。
github actions環境でMac版をビルドできるかなどもチェックできると思います。
(もし未知の原因でrevertすることになった場合大変かもですが。。)

もしくはcpp-libraryブランチのように、ブランチを分けておいてそこにプルリクエストを送っていき、完成したらmainにマージでも良いかもです。
こっちの方法はmainとのコンフリクト処理が大変という課題があります。
(個人的にはわかりやすいのでmainマージだと嬉しいです。バージョンアップもまだ結構先になりそうですし)

@PickledChair
Copy link
Member

ちょっと悩ましいですが、 @.Hiroshiba さんのおっしゃる通り

エンジン側の対応が必要な件は、製品版coreのプレリリースがあったほうがやりやすそうに感じました。

というのはあると思うので、コンフリクトが小さいうちに main にマージするのが良さそうな気がしてきました。今までのペースのリリースだと色々と作業が煩雑になるリスクが高いですが、

バージョンアップもまだ結構先になりそうですし

ということであれば、エンジン側も少し時間をかけて対応できそうです。

ということで、マージ作業をします……!

@PickledChair
Copy link
Member

x64 の Ubuntu 18.04 だけ JSON のパースに失敗してテストが落ちてますね。どうしてでしょうか……?

@Hiroshiba
Copy link
Member

本当ですね・・・きっと関係なさそうですがre runしてみます。

@Hiroshiba
Copy link
Member

エラー内容は下記らしいです。

Traceback (most recent call last):
  File "/home/runner/work/voicevox_core/voicevox_core/tests/core_test.py", line 35, in test_metas
    core.initialize(False)
  File "/home/runner/work/voicevox_core/voicevox_core/core/_core.py", line [58](https://github.com/VOICEVOX/voicevox_core/runs/5661133549?check_suite_focus=true#step:19:58), in initialize
    raise Exception(lib.last_error_message().decode())
Exception: JSON parser raise exception: [json.exception.parse_error.101] parse error at line 19, column 1: syntax error while parsing value - invalid literal; last read: '"0.0.1"<U+000A>  }<U+000A>]<U+000A>b'; expected end of input

最後の部分が '"0.0.1"<U+000A> }<U+000A>]<U+000A>b'になってる・・・? bが変なのかな・・・

ちなみに実際のmetas.jsonの最後はこんな感じです

    "version": "0.0.1"
  }
]

@Oyaki122
Copy link
Member Author

このコミットでおそらく解決しています

原因は、ファイルを直接読み込んでいるためNULLポインタが末尾にあることが保証されていないにも関わらず、文字列としてjson::parseに読み込ませたことでバッファオーバーランし、たまたまメモリ上に存在した'b'を読み込んだためと思われます
そのためイテレータ用の読み込ませ方に変更しました

マージ直前にご迷惑をおかけして申し訳ありません

@PickledChair
Copy link
Member

PickledChair commented Mar 23, 2022

修正ありがとうございます! nlohmann::basic_json::parse 関数に渡される metas.json のデータが NULL 終端されてないのを意識していなかったのが原因でしたね( https://json.nlohmann.me/api/basic_json/parse/ に仕様が書いてありましたね。こちらも見落としていました、すみません!)。これでマージしてみます。

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.

5 participants