-
Notifications
You must be signed in to change notification settings - Fork 118
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
[Rust]onnxruntimeをrust版voicevox_coreに導入 #135
[Rust]onnxruntimeをrust版voicevox_coreに導入 #135
Conversation
@@ -10,14 +10,17 @@ use std::sync::Mutex; | |||
* これはC文脈の処理と実装をわけるためと、内部実装の変更がAPIに影響を与えにくくするためである | |||
*/ | |||
|
|||
#[repr(C)] | |||
#[repr(i32)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enumの場合はrepr(i32)が正しいらしいので修正
crates/voicevox_core/Cargo.toml
Outdated
derive-getters = "0.2.0" | ||
derive-new = "0.5.9" | ||
once_cell = "1.10.0" | ||
onnxruntime = { git = "https://github.com/qwerty2501/onnxruntime-rs.git", version = "0.0.16" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえず私のリポジトリにforkしたものを参照するようにしました
[lib] | ||
name = "core" | ||
crate-type = ["cdylib"] | ||
|
||
[dependencies] | ||
anyhow = "1.0.57" | ||
cfg-if = "1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directmlでbuildするかどうかで出力されるコードを変えたかったので条件わけしやすいように導入した
[lib] | ||
name = "core" | ||
crate-type = ["cdylib"] | ||
|
||
[dependencies] | ||
anyhow = "1.0.57" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
内部で発生したエラーをError内部のsourceとして保持するために導入
7395664
to
a53b00b
Compare
3d876c9
to
93062cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
正直全然ちゃんとわかってませんが、問題がありそうな箇所は見当たらなかったのでLGTM!!
derive-getters = "0.2.0" | ||
derive-new = "0.5.9" | ||
once_cell = "1.10.0" | ||
onnxruntime = { git = "https://github.com/qwerty2501/onnxruntime-rs.git", version = "0.0.17" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コミットIDの指定ってできそうでしょうか👀
指定しておくと、今後qwertyさんがご自身のonnxruntime-rsを気軽に修正できそうだなと思い。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コミットID指定もできたはずですが、version指定のほうがやりやすいのでこっちにさせていただいて良いでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コミットID指定だとバージョンアップするたびにコミットIDを調べないといけないのでちょっと面倒です
version指定だとonnxruntime-rs projectのcargo file内のversionを更新する必要がありますが、こっちのほうが使う側は特に何も考えずにインクリメントればよいので楽かなと
あとversionの更新はきちんとやるべきだと思いますし
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです!バージョンが良さそうに感じました!
(変更があまり発生しなさそうになったらぜひvoicevox側でもメンテしたいですね…!)
Co-authored-by: Hiroshiba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! コードが見やすく、とても参考になります……!
Approve したのですが、いくつか議論目的のコメントが残っていると思うので、それらの様子を見てからマージになるかと思います。よろしくお願いします。
LGTM!! |
* Statusを仮実装した * get_supported_devices を仮実装した * エラーの名前とsourceの修正 * initialize関数を仮実装した * get_supported_devicesが動作するかどうか確認するためのテストを追加 * lint errorを修正した * version up onnxruntime-rs to 0.0.17 * TODOコメント追加 * Update crates/voicevox_core/Cargo.toml Co-authored-by: Hiroshiba <[email protected]> Co-authored-by: Hiroshiba <[email protected]>
内容
onnxruntimeをvoicevox_core rustに導入するPRです
関連 Issue
refs #128
その他
onnxruntimeの機能がちゃんと呼べてるかは get_supported_devicesのテストコードで確認している
initialize関数のボリュームが思っていた以上に重かったので、TODOコメントを残して実装は途中までにしてある