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

mutabilityとasyncnessを仕上げる #553

Merged
merged 21 commits into from
Aug 17, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jul 28, 2023

内容

関連 Issue

Resolves #552.
ref #545

その他

@qryxip qryxip mentioned this pull request Jul 28, 2023
3 tasks
@qryxip
Copy link
Member Author

qryxip commented Jul 28, 2023

今この実装を自分で見直しているのですが、思い付くまま書き殴った設計そのままなので、その意味でも #552 ベースで書き直した方が良いかもと思ってしまいました。

(追記) 「余計な情報を持たないようにする」という方針は心掛けていて、それは体現できていると思うので、コンセプトについてはそのままにしたいと思います。

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.

実装読みました!
コンセプトとしては、Inference Coreには一旦手を加えず、Statusをデータ周りと推論周りで二層構造にする形かなと感じました!
そういう目的であるということが分かりやすい構造体名になっていると嬉しそう?

あとInference CoreからStatusの関数を2回以上叩いている関数がちょくちょくあって、これもMutexにしとかないといけないはず。status.validate_speaker_idとか。
まあこっちをMutexにするのは面倒なので、Inference Coreからstatusを2回呼ぶような設計にしなければ一旦大丈夫かなと思いました!

crates/voicevox_core/src/status.rs Show resolved Hide resolved
crates/voicevox_core/src/status.rs Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

こちらのブルリクエストのデータ構造みたいなのを書き下してみました!!
ちょっとこのプルリクエストで議論することかどうか微妙ですが、StatusはInference Coreと集約できそうなこと、Synthesis EngineはSynthesizerのimplそして実装していくのが将来的には良さそうかなとか思いました!!

flowchart LR
	276662["session.run"] --o 672471(["Onnx Session (Mutex)"])
	115090["Inference Core"] --o 605462["Status"]
	939501["Syntthesis Engine"] --o 115090
	132055["Synthesizer"] --o 939501
	939501 --o 707071(["OpenJTalk"])
	515265["metas"] --o 909525["metas"]
	605462 --o 326867["Loaded Models (Mutex)"]
	909525 --o 343936["metas"]
	343936 --o 628672["metas"]
	999221["predict"] --o 635888["predict"]
	635888 --o 276662
	927541["synthesis"] --o 394870["synthesis"]
	394870 --o 635888
	699899["load_voice_model"] --o 209723["load_model"]
	209723 --o 367614["load_model"]
	367614 --o 629577["insert"]
	678674(["metas"]) --> 684359(["metas"])
	367614 --o 920120["Voice Model"]
	163279(["Onnx Model"]) --> 672471
	subgraph 115090["Inference Core"]
		909525
		635888
		209723
	end
	subgraph 132055["Synthesizer"]
		515265
		999221
		927541
		699899
	end
	subgraph 326867["Loaded Models (Mutex)"]
		628672 --o 684359
		629577 --o 174035["Loaded Model"]
		subgraph 174035["Loaded Model"]
			684359
			672471
		end
	end
	subgraph 605462["Status"]
		276662
		343936
		367614
	end
	subgraph 920120["Voice Model"]
		163279
		678674
	end
	subgraph 939501["Syntthesis Engine"]
		394870
	end
Loading

@qryxip
Copy link
Member Author

qryxip commented Jul 29, 2023

  • Merge branch 'main' into HEAD
  • Criterionによるベンチ(decode-with-gpu)の抹消 (ce2f8c0)
  • num_gpu_sessionsオプションの抹消 (47a77ee)
  • レビューの反映

@qryxip
Copy link
Member Author

qryxip commented Jul 29, 2023

@qryxip qryxip marked this pull request as ready for review August 6, 2023 19:14
@qryxip
Copy link
Member Author

qryxip commented Aug 6, 2023

@qryxip
Copy link
Member Author

qryxip commented Aug 7, 2023

このPRの

モデル実行部分以外のCPU-boundな部分もtokio::task::spawn_blockingで包む

ですが、結構小刻みで包むことになってしまいますね。
#553 (comment)の構造改革のときに操作全体を丸ごとspawn_blockingで包めるように設計する、というのがいいかもしれません。

@qryxip
Copy link
Member Author

qryxip commented Aug 11, 2023

コンフリクト解消が適当だったので同じ宣言が二つできていてwarningが出てい
た。
@qryxip
Copy link
Member Author

qryxip commented Aug 12, 2023

@qryxip qryxip requested a review from Hiroshiba August 13, 2023 07:02
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.

とりあえず大まかな部分を見てみました!
以前見た時と大枠は全く変わってない感じという理解であってそうでしょうか?

この認識で問題なければ、Async周りとかをしっかりめにレビューしたいと思います!

crates/voicevox_core/src/status.rs Outdated Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Aug 15, 2023

以前見た時と大枠は全く変わってない感じという理解であってそうでしょうか?

はい。 #551 のマージはしましたが、大きく変えないようにしました。

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です!!
コメントしたところだけ変更もしくは再コメントいただけると!!!

これはこのプルリクエストで追加する必要はなく、ちょっとしたコメントなのですが、
Async/Mutexに関して、望んだ実装になっているかどうか(例えば同じモデルの同じセッションは同時に実行できないとか)をテストできると嬉しいのかなと思いました!
ソースコードから判断しようとすると間違えそうなので・・・。

pub fn predict_duration_session_run(
/// # Panics
///
/// `self`が`model_id`を含んでいないとき、パニックする。
Copy link
Member

Choose a reason for hiding this comment

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

(判断メモです)
パニックになってしまうのは結構きつい制約かなと思ったのですが、ユーザーがmodel_idを直接指定して実行する機会がないのでパニックで良さそうな気がしました!

})
.ok_or(Error::InvalidStyleId { style_id })?;
Copy link
Member

Choose a reason for hiding this comment

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

多分このプルリクエストの範囲じゃないと思うんですが、InvalidStyleIdというよりはNonExistStyleIdかもと思いました。

@qryxip
Copy link
Member Author

qryxip commented Aug 16, 2023

@Hiroshiba
Copy link
Member

あとエラーの形が調整完了すればマージかなと!

crates/voicevox_core/src/result_code.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/error.rs Show resolved Hide resolved
crates/voicevox_core/src/status.rs Outdated Show resolved Hide resolved
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です!!

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!!!

Async系のテストがあるとかっこよさそうですが、コンパイル言語だとモックとかも難しそうだから相当難しそう。

@Hiroshiba
Copy link
Member

問題ないと思うのでマージします!

あ、以前描いたmarmaid、共有します。よかったらどうぞ 🙏

Details
flowchart LR
	276662["session.run"] --o 672471(["Onnx Session (Mutex)"])
	115090["Inference Core"] --o 605462["Status"]
	939501["Syntthesis Engine"] --o 115090
	132055["Synthesizer"] --o 939501
	939501 --o 707071(["OpenJTalk"])
	515265["metas"] --o 909525["metas"]
	605462 --o 326867["Loaded Models (Mutex)"]
	909525 --o 343936["metas"]
	343936 --o 628672["metas"]
	999221["predict"] --o 635888["predict"]
	635888 --o 276662
	927541["synthesis"] --o 394870["synthesis"]
	394870 --o 635888
	699899["load_voice_model"] --o 209723["load_model"]
	209723 --o 367614["load_model"]
	367614 --o 629577["insert"]
	678674(["metas"]) --> 684359(["metas"])
	367614 --o 920120["Voice Model"]
	163279(["Onnx Model"]) --> 672471
	subgraph 115090["Inference Core"]
		909525
		635888
		209723
	end
	subgraph 132055["Synthesizer"]
		515265
		999221
		927541
		699899
	end
	subgraph 326867["Loaded Models (Mutex)"]
		628672 --o 684359
		629577 --o 174035["Loaded Model"]
		subgraph 174035["Loaded Model"]
			684359
			672471
		end
	end
	subgraph 605462["Status"]
		276662
		343936
		367614
	end
	subgraph 920120["Voice Model"]
		163279
		678674
	end
	subgraph 939501["Syntthesis Engine"]
		394870
	end

@Hiroshiba Hiroshiba merged commit f4868ac into VOICEVOX:main Aug 17, 2023
45 checks passed
This was referenced Oct 29, 2023
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.

mutabilityとasyncnessを仕上げる
2 participants