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

新クラス設計API #370

Merged

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented Jan 9, 2023

関連 Issue

refs #280,#280 (comment)

Copy link
Contributor Author

@qwerty2501 qwerty2501 left a comment

Choose a reason for hiding this comment

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

とりあえず実際のコード見たほうがイメージ付きやすいかと思ったのでつくりました
全体的にちょっと長めの命名にしてます

crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/publish.rs Outdated Show resolved Hide resolved
pub struct Device {}

/// スピーカーId,スタイルIdから音声合成モデルIdを取得する
pub fn get_model_id(speaker_id: &SpeakerId, style_id: &StyleId) -> Result<SpeechSynthesisModelId> {}
Copy link
Member

Choose a reason for hiding this comment

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

これ便利そうですね!
たぶんModelを作成して初めてglobalメモリ上にmetasのデータが乗って、そこからmodel_idを引っ張ってくる形ですよね。
となると、Model.from_idとこっちのどっちが先なのかみたいな問題が出てきそうだなと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

いや違います。これはビルトインのmodelを読み込む際にユーザーがmodel idを知るすべがないのでとりあえず作ったものですね
なのでmodel作成前に呼ばれることを想定しており、modelがビルトインじゃなくなったらこの関数は消そうかと思ってます

Copy link
Member

@Hiroshiba Hiroshiba Jan 9, 2023

Choose a reason for hiding this comment

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

あーなるほどです!であれば問題はなさそうです。

(まあビルトインのモデルはunloadする方法はなくても良い派なので、この関数は最初からなしでも良いかも!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

仮にビルトインのmodelはunload提供しなかったとしても全loadせずに個別にビルトインのmodelを読み込みたい場合に必要ではないでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

それは必要だと思います!VVM対応したら自動的にその需要は満たせるかなと。

つまり(ちょっと話が進んじゃうのですが)こういうのを考えてます。
ビルトインも2種類あると思います。
1つが今のCOREみたくVVMがなく勝手にloadされてるパターンですが、こちらはunloadもloadも提供しなくて良いかなと。
もう1つが製品版のVVMファイルを全部loadされてるパターンで、これは「勝手にloadしない」ようにできる制御フラグを付けつつ、あとはVVMをload/unloadしてもらえればみたいな感じでいます。

Copy link
Member

@Hiroshiba Hiroshiba Jan 17, 2023

Choose a reason for hiding this comment

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

VVMも全部入ってるものの提供を考えてました!
まあ個別にするかは置いといて、同ディレクトリにあるvvmファイルを全部読み込むかどうか、みたいなオプションになるんじゃないかなーと思っています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

それか今と同じでmodelsディレクトリ配下にあるVVMファイルを読み込むとかですかね。
しかしそうなるとファイルパス指定のAPIの価値があまりなくなってしまいますね
個別に読み込む際に使えそうですが、ユーザーがどのVVMを読み込めば目的のスピーカーを使うことができるか知る手段を提供するつもりでいますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

目的のスピーカーというよりかは目的のスタイルですかね

Copy link
Member

Choose a reason for hiding this comment

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

modelsディレクトリでも良いと思います!(そっちのほうが良さそう)

知る手段

重要なことですが、良い方法が思いつかないですね・・・・・。
全metas.jsonを書き下したREADME.mdを作るとか・・・・・・。
せめて検討がつくよう、わかりやすいファイル名にしてあげたいですね・・・。

そうなるとファイルパス指定のAPIの価値があまりなくなってしまいますね

現ニーズは現APIなので、この視点だとパス指定周りとかは価値が薄いとは思います。
でもまあVVMを抜き差しできれば、例えば差分アップデートなど別の仕様も思いつけると思います!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_model_idについては削除

@qwerty2501
Copy link
Contributor Author

今議論されているものについてAPIを最新に変更しました
とりあえずpublish.rsに書いていますが、実際の実装ではファイルを分けて実装する形になると思います

@Hiroshiba
Copy link
Member

議論ありがとうございました!!
あと大きなのはVoiceModelファイルのファイル形式ですかね…?

@qwerty2501
Copy link
Contributor Author

そうですね。そこ決まったらとりあえずの仮実装はできそうです
実際に実装するのは他のPRのmergeされ具合を見てからになりそうですが

@qwerty2501
Copy link
Contributor Author

Rust化したときにはC APIしかなかったから同期処理だけにしてたけど、Python,将来的にはRustも公開することを考えるとRust実装とPython実装の部分についてはasync化できるところ(VVMファイルをオープンする処理とか)はasync化したほうがよいかも? 🤔

@Hiroshiba @qryxip 意見聞かせてもらえますか?

@Hiroshiba
Copy link
Member

良い設計がまだ浮かんで無いですが、面白いと思います!
オプショナルな立ち位置なのかなと思いました。

@qryxip
Copy link
Member

qryxip commented Jan 24, 2023

RustのasyncはPythonのasyncに変換できたりしますし、そこからブロッキングなAPIに派生させることも普通にできるため、ありかとは思います。

@qwerty2501
Copy link
Contributor Author

APIを一度公開してしまうとあとから変えるのは難しいくtryするならいましかなさそうなのでやってみます。
decodeなどIOではないが時間がかかりそうな処理についても非同期にしたほうがいいですかね?

@Hiroshiba
Copy link
Member

たしかにAPI変更に関してはおっしゃる通りかもです。
onnxruntimeの実行は同時にできるのかかなり怪しい気がしています。
仕様上問題ないのであればぜひという感じです!

@qwerty2501
Copy link
Contributor Author

onnxruntimeもそうですが、openjtalkも同時に実行すると壊れてしまうので今と同じように全体的にロックをかける必要はありそうですね
将来的にはそのあたりもロックなしにできるとよいですが

@qryxip
Copy link
Member

qryxip commented Jan 24, 2023

そこはまあ!Syncの制約に任せていいんじゃないですかね。
Environmentouroborosとかで抱えるか、VoiceSynthesizerの複数生成を禁じるかして。
Environmentの中身はArc<Mutex<_>>でしたね。ouroborosとかは不要でした。

@qwerty2501
Copy link
Contributor Author

そもそもEnvironmentについてはnew_session_builderでimmutable参照されてるだけなのでどちらにしろ不要かも

@qryxip
Copy link
Member

qryxip commented Jan 25, 2023

そこはまあ、今(多分時間差で)↑のコメントをeditしましたがonce_cellに置いたままでもよいので、使いまわせるなら使いまわした方がいいんじゃないでしょうか。

@qwerty2501 qwerty2501 force-pushed the feature/new_class_design branch from db09d19 to b3a8165 Compare January 30, 2023 11:56
@qryxip qryxip mentioned this pull request Feb 5, 2023
3 tasks
@qwerty2501
Copy link
Contributor Author

qwerty2501 commented Feb 7, 2023

@Hiroshiba engine互換APIためにstyle_idとvvmとは別にstyle_idとvvmファイルのパスの対応情報をまとめたファイル
と、すべてのmetas情報をまとめたmetaファイルが必要そうです

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 7, 2023

( ちょっと記憶が曖昧で変なことを言っちゃうかもしれません・・・。)

先に後者のこちらから

すべてのmetas情報をまとめたmetaファイルが必要そうです

これに関してはmetaファイルを用意しなくても、全VVMをloadしたSynthesizerのmetasを使えば良いのかなと思ってました・・・が、全modelをloadするので重そうですね・・・。

ファイルを用意しなくても、全VVMから全Modelを作って全Modelのmetasを集約する手もありそう・・・?
この形だとコード的に大変な感じでしょうか。

style_idとvvmとは別にstyle_idとvvmファイルのパスの対応情報をまとめたファイル

あーーー。これは必要になったstyle_idのModelだけloadするため、ですよね。
盲点でした。今の仕様だと、predict_durationやdecodeは各Modelに含まれるstyle_idを意識しなくて良い一方、loadはどのModelがどのstyle_idを持っているか把握する必要があって不便かもです・・・。

Modelを参照するだけのset関数と、style_idを指定して実際に読み込むload関数をSynthesizerに持たせるのはどうでしょうか。。
こうすればloadすることなく全Modelのmetasを得ることができて、style_idとvvmファイルの対応を別途用意する必要もなくなるかも・・・・・・?

@qwerty2501
Copy link
Contributor Author

qwerty2501 commented Feb 8, 2023

ファイルを用意しなくても、全VVMから全Modelを作って全Modelのmetasを集約する手もありそう・・・?
この形だとコード的に大変な感じでしょうか。

可能ですが互換性守るならmetasはdll読み込み時にすでに用意できてる必要があるのでdll読み込みが相当遅くなります

Modelを参照するだけのset関数と、style_idを指定して実際に読み込むload関数をSynthesizerに持たせるのはどうでしょうか。。
こうすればloadすることなく全Modelのmetasを得ることができて、style_idとvvmファイルの対応を別途用意する必要もなくなるかも・・・・・・?

前のやり取りだと新APIは読み込んでいるmetasのみ返す仕様にするとのことだったのですが常に全てのmetasを返すようにするということでしょうか?

@qwerty2501
Copy link
Contributor Author

ちょっと対応できそうかもなのでやってみます

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 8, 2023

前のやり取りだと新APIは読み込んでいるmetasのみ返す仕様にするとのことだったのですが常に全てのmetasを返すようにするということでしょうか?

あ、違います!
Synthesizerのモデル読み込み関数をなくし、モデル登録関数と読み込み関数を分けるニュアンスです。
実際のコードに近い形で書くと、load_model(model)をset_model(model)とload(style_id)に分ける感じを想像しています。

狙いは2つあって、1つ目の理由が読み込まなくてもmetasが分かるようにするためです。
setしたモデルからmetasを集約する仕様にすればできるはず。

もう1つの狙いは↓の解消です。
音声合成部分と読み込み部分でModelの扱いが同じでないのを解消する狙いです。

今の仕様だと、predict_durationやdecodeは各Modelに含まれるstyle_idを意識しなくて良い一方、loadはどのModelがどのstyle_idを持っているか把握する必要があって不便かもです・・・。

@qwerty2501
Copy link
Contributor Author

qwerty2501 commented Feb 9, 2023

Synthesizerのモデル読み込み関数をなくし、モデル登録関数と読み込み関数を分けるニュアンスです。

公開APIとしてはload_model関数のみにしたほうが良いと思います。

狙いは2つあって、1つ目の理由が読み込まなくてもmetasが分かるようにするためです。
setしたモデルからmetasを集約する仕様にすればできるはず。

これはVoiceModelでmetasを確認できるので不要では

もう1つの狙いは↓の解消です。
音声合成部分と読み込み部分でModelの扱いが同じでないのを解消する狙いです。

これはunloadの振る舞いが不自然になる問題があるので解消しきれないと思います

@qwerty2501
Copy link
Contributor Author

@Hiroshiba 過去のやり取りでもunloadとの整合性が取れなくなる点について言及されてますね
#280 (comment)

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 12, 2023

これはunloadの振る舞いが不自然になる問題があるので解消しきれないと思います

あああ・・・・・・。すみません、これ失念していました・・・。

うーん。unloadややこしい問題があるのでsetとloadに分けるのは難しいかなと思いました!
気持ちとしては、全部入りのSynthesizerがVoicevoxApiと同じようなインターフェイスになるのが良いのかなと(なんとなく)思っての提案でした。

@qwerty2501 qwerty2501 force-pushed the feature/new_class_design branch 2 times, most recently from a83538b to c3a0a76 Compare February 23, 2023 16:38
@qwerty2501 qwerty2501 changed the title 新クラス設計を元にしたAPIたたき 新クラス設計API Feb 23, 2023
@qwerty2501 qwerty2501 marked this pull request as ready for review February 23, 2023 18:47
@qryxip
Copy link
Member

qryxip commented May 21, 2023

コンフリクトを解消しました。マージ可能です。

つきましてはこのタスクリストに追加で入れるべき内容が無いかだけ、どなたかに見てもらえたらと思っています。
#280 (comment)

@Hiroshiba
Copy link
Member

@qryxip ざっと見た感じ問題ないのかなと思いました!!
おそらくリリースは7月とかになると思うので、破壊的変更はしちゃっても大丈夫なので「以前のAPIをdeprecatedなものとして残す」はオプショナル的なタスクかなと思いました!!

@Hiroshiba
Copy link
Member

Hiroshiba commented May 22, 2023

いったんマージさせていただきます!
@qryxip さん、そして @qwerty2501 さん、ほんとにありがとうございました!!!

@Hiroshiba Hiroshiba merged commit fb24f4f into VOICEVOX:project-vvm-async-api May 22, 2023
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Jun 13, 2023
@qryxip qryxip mentioned this pull request Jul 22, 2023
69 tasks
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.

onnxruntimeのSessionを解放してGPUメモリをリフレッシュできる仕組みを作る
7 participants