Skip to content

[Rust]load_metas,metasを実装した #140

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

Merged
merged 12 commits into from
Jun 3, 2022

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented May 25, 2022

内容

Status.load_metasとそれに関連する dllで exportする用のC API関数であるmetas関数の実装を行った

関連 Issue

refs #128

その他

initialize関数内部でstatus.load_metasを呼び出すようにした
#139 が mergeされてからレビューすること

@qwerty2501 qwerty2501 changed the title load_metas,metasを実装した [Rust]load_metas,metasを実装した May 25, 2022
@qwerty2501 qwerty2501 force-pushed the feature/implements_load_metas branch from 46513b7 to 463edcf Compare May 26, 2022 02:27
@Hiroshiba
Copy link
Member

手元の環境(windows 10)でcargo testしたところ、process didn't exit successfully: C:\Users\hihok\Github\voicevox_core\target\debug\deps\core-0f57f48f4ee7074b.exe . (exit code: 0xc000007b)となりました。
よくわかっていませんが、dll周りのエラーっぽく、target/debug/deps/のとこにonnxruntime.dllを配置すると動きました。

actionsのテストは回っているっぽいので見たところ、なぜかテストがfilterされてるっぽい?です。
image

@qwerty2501 qwerty2501 force-pushed the feature/implements_load_metas branch from 463edcf to eba2324 Compare May 28, 2022 21:14
@qwerty2501 qwerty2501 marked this pull request as ready for review May 28, 2022 21:22
@qwerty2501
Copy link
Contributor Author

とりあえずCIでテスト結果が正確に反映されるようにしたのですがWindows server 2022だと失敗してますね。あとUbuntu18でも何故か失敗してます

@qwerty2501 qwerty2501 force-pushed the feature/implements_load_metas branch from 5c9819c to f4d691a Compare May 28, 2022 22:03
@qwerty2501 qwerty2501 marked this pull request as draft May 28, 2022 22:04
@qwerty2501 qwerty2501 force-pushed the feature/implements_load_metas branch from f4d691a to 52ec149 Compare May 28, 2022 22:13
@qwerty2501
Copy link
Contributor Author

Windowsの方はやはりdll読み込みで失敗してるようです

@qwerty2501
Copy link
Contributor Author

@Hiroshiba 参考までに正しいバイナリファイルのURLってどれ使いましたか?

@Hiroshiba
Copy link
Member

@qwerty2501 crago buildするとtargetの下にdepsディレクトリができてそこにクレートたちが入ると思うのですが、そこのonnxruntime-sysディレクトリの中にonnxruntimeがダウンロードされているのでそれを使いました!
だからたぶん、onnxruntime-sys内のdllをコピーするか環境変数で指定すればOKな気がしています。

@PickledChair
Copy link
Member

PickledChair commented May 29, 2022

@qwerty2501

あとUbuntu18でも何故か失敗してます

Ubuntu 18.04 と 20.04 の場合で、rust-cache が同じキャッシュを使用してしまっているからと推測しました。 https://github.com/VOICEVOX/voicevox_core/actions/runs/2402391185 では両者ともキーとして以下が使用されていました。

Using keys:
    v0-rust-rust-test-1.61.0-x86_64-unknown-linux-gnu-fe5b13d681f2-0807e0f7beda03015d4f
    v0-rust-rust-test-1.61.0-x86_64-unknown-linux-gnu-fe5b13d681f2

この結果、18.04 の方で対応できない glibc のバージョンの設定があったか何かでこのエラーが出たのではないか、と考えています。念の為、自分のリポジトリでも CI を回してみました https://github.com/PickledChair/voicevox_core/actions/runs/2404701848 。初回はキャッシュがないので 18.04 でもビルドに成功していますが、「他のジョブで同名のキャッシュを作っているからキャッシュが保存できない」という警告が出ていました。

対応策としては、 https://github.com/Swatinem/rust-cache#inputs によるとキャッシュのキーに追加の文字列を付け加えられそうなので、それで解決できそうです(OSのバージョンなどを付け加えるなど)。ちょっと試してみて、うまくいったら PR を出そうと思います(あるいは、この PR 内で解決しますか?)。

@qwerty2501
Copy link
Contributor Author

cargo testがCI上でちゃんと実行されてないのもこのPRでやっちゃったんですが分けたほうが良かったですかね

@Hiroshiba
Copy link
Member

ちょっと考えてたのですが、「rust移行できそうかをなるはやで検証する」のを目的に動くのが良いのかなと思いました!
となると、テスト実装もついでにやっちゃって大丈夫ですし、なんなら別OSで動かないのはいったんパスして進んじゃっても良いのかなと感じました!

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!

include_str!良いですね。バイナリも簡単に埋め込みできそう。

@qwerty2501
Copy link
Contributor Author

Ubuntu18.04の方は直しました
Windowsについてですが、Windows2019のほうが成功しているのがちょっと気になりますね。
単純にコピーして良いものかどうか・・・

@qwerty2501 qwerty2501 force-pushed the feature/implements_load_metas branch from 0ab910c to 49fec5b Compare May 30, 2022 00:11
@qwerty2501
Copy link
Contributor Author

qwerty2501 commented May 30, 2022

@Hiroshiba いくつか調査と更新を加えました。

まずonnxruntime-rsのビルドに clang及びllvm が必要になりました。これは元のonnxruntime-rsだとC headerから生成したrustコードをgit repository上で管理していたのですが、それだとheaderが新しいバージョンになった際に各OSごとのコード生成を行わなければならないからです。その際に実際にそのOSで生成する必要があるためメンテナンスが困難(特にMac)になるため、ビルド時に各自で自動的にrustコードを生成するようにしてそれを参照するようにしました。clangとllvmはその際に必要になります。

次にテスト実行時にdllが読み込まれない点についてですが、 どうやら ここ でその設定が行われているようです。 rustc-link-searchに加えられたディレクトリはWindows環境だと PATH環境変数に含まれるため、dllが読み込まれるようです
https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-search

These paths are also added to the dynamic library search path environment variable if they are within the OUT_DIR. Depending on this behavior is discouraged since this makes it difficult to use the resulting binary.

そのためdllが読み込めてないのは何かがおかしいのでコピーするのはちょっと待ったほうが良さそうです
またdepsディレクトリは使うべきではないとのことです

clangとllvmについてですが恐らくopenjtalkをRustでwrapする際にも必要かと思われるのでそこまで痛手にはならないはずです

追記:
clangとllvmについて、なんとかインストールしないでできないか考えてみます

@Hiroshiba Hiroshiba marked this pull request as ready for review May 30, 2022 17:38
@Hiroshiba
Copy link
Member

clangとllvmが必要な件、一旦了解です!共有ありがとうございます。
なくせたほうが参入しやすそうなので、依存を外せそうであればやっぱり外したほうが良さそうに感じました。

windowsに関しても調査ありがとうございます。
いったんissueの方に情報を集約した方が良いのかなと感じました。もしよければisssue側にコメントの追記をお願いできると嬉しいです!

テストは全部通らないかもですが、いったんマージできればと思っています。

@Hiroshiba
Copy link
Member

clangとllvmが必要

まとめると、

  • onnxruntime-rsのonnxruntimeのバージョンが低いのでアプデしたい
  • そのためにはOSごとのrustコードが一部必要で、そのコード生成にclangとllvmが必要
  • mac/win/linuxの全rustコードをonnxruntime-rs側で用意するのは大変なので、いったんローカルPCにinstallする際にコード生成するようにする

ということでしょうか。
たしかに複雑度は一旦上がりますが、全対応をqwertyさんにおまかせするのは負荷が高そうなので、その形が良いのかなと思いました!
もしうまいことビルドできなかったりしたら早めになんとかするのを検討したいかもです。

@qwerty2501
Copy link
Contributor Author

issue記載しておきました。

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.

キャッシュのキーに関して一点質問させていただきました。それ以外は大丈夫そうです!

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!

@PickledChair PickledChair merged commit 34f88f3 into VOICEVOX:rust Jun 3, 2022
qwerty2501 added a commit to qwerty2501/voicevox_core that referenced this pull request Jul 23, 2022
* load_metasを実装した

* Statusのnew関数テストを追加した

* load_metasのテストを追加した

* なぜか.が入っていたため削除

* assertでresultの内容を表示するようにした

* output err string

* エラーを詳細に表示するように修正

* cache keyを設定した

* onnxruntime versionup

* Windows2019向けのlibclang install

* onnxruntime version up

* キャッシュ制御を環境変数で行うことをやめた
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