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.

Sign up for GitHub

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

Jump to bottom

voicevox-ortとして開発する #2

Merged
merged 3 commits into from
May 21, 2024
Merged

voicevox-ortとして開発する #2

merged 3 commits into from
May 21, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Feb 11, 2024 •
edited
Loading

内容

v2.0.0-rc.0を対象に、以下の変更を加えます。

  • パッケージ名（crates.ioにアップロードするときの名前）はvoicevox-ortとvoicevox-ort-sysとする
    • "crate name"（Rustの実装内での名前）はortとort_sysのまま
  • onnxruntime-builderでビルドしたlibonnxruntimeを利用する
  • 本ライブラリはビルド時に~/.cache/ort.pyke.io（Linuxの場合）にlibonnxruntimeをダウンロードしてそれをリンクするが、それを~/.cache/voicevox_ortにする
  • directmlとcuda以外のExecution Providerをとりあえず拒否
  • libonnxruntime本体から流れてくるログの扱いを、VOICEVOXに合わせる

関連 Issue

スクリーンショット・動画など

その他

@qryxip
Copy link
Member Author

qryxip commented Feb 11, 2024

Wait, ort's committers involved in this PR...?

image

@qryxip qryxip mentioned this pull request Feb 11, 2024
Merged
Hiroshiba
Hiroshiba reviewed Feb 19, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM…なのですが、たぶん追従とボイボ用の変更のコミット（PR）を分けておいたほうが良さそうに感じました。
あとあとなんで変更したのか目的を推察したい時とかが出てきたときに大変になりそうだな〜と。
あと追従はなんとなくsquashしない方が良さそう…？

あ、たぶんrc0に追従したあとこのPRをマージするのが一番簡単そう？

@qryxip
Copy link
Member Author

qryxip commented Feb 19, 2024

あ、たぶんrc0に追従したあとこのPRをマージするのが一番簡単そう？

ですね。mainをrc0にした後squashというつもりでした。
(mainの更新はPRをマージする直前でいいんじゃないかと考えた結果、今このPRにdecahedron氏とかが巻き込まれているわけですが)

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 19, 2024 •
edited
Loading

あっなるほど、たぶんお見合い状態になってますね！
mainブランチのマージもPRにしたほうが良いと思うので、他のリポジトリだとこんな感じでPR作ってFF mergeしてます。

PRお願いできると助かります！！

@qryxip
Copy link
Member Author

qryxip commented Feb 19, 2024

VOICEVOXとしてのコミットはまだ0でfast-forwardなので、mainをそのままrc.0に移動すればよくないでしょうか…?

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 19, 2024 •
edited
Loading

あ、なるほどです！
僕がやるにはどうするのが正しいか調べないとなので、詳しいqryxipさんからPRもらった方が確実で早いのではとか思いました…！
お忙しかったらやります🙏
（mainにpushするのはなんか危ないので、僕がやる場合もPR作ります）

@qryxip qryxip mentioned this pull request Mar 2, 2024
Merged
@qryxip qryxip marked this pull request as draft April 13, 2024 15:47
@qryxip qryxip marked this pull request as ready for review April 14, 2024 08:22
@qryxip qryxip requested a review from Hiroshiba April 14, 2024 08:22
@qryxip qryxip marked this pull request as draft April 14, 2024 08:25
@qryxip qryxip marked this pull request as ready for review April 15, 2024 17:49
@qryxip
Copy link
Member Author

qryxip commented Apr 15, 2024 •
edited
Loading

Windowsだとなんかonnxruntime.dllが壊れているっぽい。正直rust-cacheが怪しそう。

assertion failed: !api.is_null()

(追記) 怪しいというより、キャッシュの形成のされかたを見るに確実にそうな気がしてきた。

@Hiroshiba 今私にwrite権限が無いので、試しにv0-rust-test-1.77.2-x86_64-pc-windows-msvc-25ef9e3d85d9-593dad0fe66fdfa9697fを消して頂けますか?

@Hiroshiba
Copy link
Member

@qryxip キャッシュ全部消しました！！

@qryxip
Copy link
Member Author

qryxip commented Apr 29, 2024

Windowsが落ちてたの、よく見たらdoctestだけ失敗してました。VOICEVOX/voicevox_core#537と同じ問題です。

ortがlibonnxruntimeを静的リンクしているのに対してvoicevox-ortは動的リンクにしているため根本的にどうにかすることはできない、ということでWindowsではdoctestを除外するようにします。
c933d60 (#2)

@qryxip qryxip marked this pull request as draft May 1, 2024 03:33
@qryxip qryxip marked this pull request as ready for review May 11, 2024 07:19
Hiroshiba
Hiroshiba reviewed May 14, 2024
View reviewed changes
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です！！

変更ありがとうございます！
１点だけコメントしました･･･！

@qryxip
Copy link
Member Author

qryxip commented May 14, 2024 •
edited
Loading

review commentが見当たらないので、どっか飛んでいってるかも…?
(あるいはブラウザの別タブに下書きが残っているか)

Hiroshiba
Hiroshiba reviewed May 16, 2024
View reviewed changes
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.

あっ本当ですね。。。 🙇
たぶんこんな感じのコメントだった記憶･･･

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

qryxip commented May 18, 2024

何故かはわかりませんがどうもpykeio@1154757によって駄目になったっぽいので、カバレッジ出す部分もif: falseにして無効化しました。
9090570 (#2)

@qryxip qryxip requested a review from Hiroshiba May 18, 2024 16:06
Hiroshiba
Hiroshiba approved these changes May 21, 2024
View reviewed changes
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！！

えーっとff-merge･･･じゃなくて良さそう！squashマージします！

@Hiroshiba Hiroshiba merged commit a2d6ae2 into VOICEVOX:main May 21, 2024
7 checks passed
qryxip added a commit to qryxip/voicevox_core that referenced this pull request May 21, 2024
@qryxip
VOICEVOX/ort#2 に追従する
93f7885
qryxip added a commit to VOICEVOX/voicevox_core that referenced this pull request May 22, 2024
@qryxip
onnxruntime-rsからortに乗り換える (#725)
5a644ca 
* onnxruntime-rsからortに乗り換える

* `--features onnxruntime/disable-sys-build-script`を消す

* ortをアップデート

* ortをアップデート

* `onnxruntimeVersion`をアップデート

* libonnxruntimeのコピー処理を更新

* ortをアップデート

* libonnxruntimeのコピー処理を更新

* ortをアップデート

* ortをアップデート

* `ort::ExecutionProvider::is_available`を使う

* `todo!`を消す

* ortをアップデート

* ortにあったAPIを使う

* ortをアップデート

* `$ORT_OUT_DIR`を削除

* ortをアップデート

* ログのフィルタを更新

* ortをアップデート

* tracingのレベルでortのログを抑える

* Minor refactor

* ortをアップデート

* Fix Cargo.lock

* Gradleのlibonnxruntimeのバージョンを更新

* ort v2.0.0-rc.1ベースに切り替える

* Gradleのlibonnxruntimeのバージョンを更新

* `with_execution_provider` → `register`

* ort v2.0.0-rc.2ベースに切り替える

* Gradleのlibonnxruntimeのバージョンを更新

* voicevox-ortを更新

* VOICEVOX/ort#2 に追従する
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qryxip @Hiroshiba