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

StyleMeta::r#typeを追加し、トークという区分を実装に導入する #761

Merged
merged 53 commits into from
May 6, 2024
Merged

StyleMeta::r#typeを追加し、トークという区分を実装に導入する #761

merged 53 commits into from
May 6, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Mar 7, 2024 •
edited
Loading

内容

#737 の下準備として、VVMがトーク用モデルセットを必ずしも持たなくてよいようにします。

manifest.jsonは次のようにします。

-  "decode_filename": "decode.onnx",
-  "predict_duration_filename": "predict_duration.onnx",
-  "predict_intonation_filename": "predict_intonation.onnx",
+  "talk_model_filenames": {
+    "predict_duration": "predict_duration.onnx",
+    "predict_intonation": "predict_intonation.onnx",
+    "decode": "decode.onnx"
+  },

(edit) その他にも、実装の各所に"talk"という名前を導入します。

関連 Issue

#581 (?)

その他

@qryxip qryxip requested a review from Hiroshiba March 7, 2024 17:10
@qryxip qryxip marked this pull request as draft March 8, 2024 15:35
@qryxip qryxip removed the request for review from Hiroshiba March 8, 2024 15:35
@qryxip qryxip changed the title VVMが持つトーク用モデルをオプショナルにする トークという区分を実装に導入し、VVMのトーク用モデルをオプショナルに Mar 8, 2024
@qryxip qryxip marked this pull request as ready for review March 8, 2024 18:51
@qryxip qryxip requested a review from Hiroshiba March 8, 2024 18:51
@qryxip
Copy link
Member Author

qryxip commented Mar 8, 2024

PRのタイトルを変更し、最初のコメントにも追記を入れました。 #737 の準備ということで、実装上でトーク用モデルを"talk"いう名前を与えるようにしました。

@qryxip qryxip marked this pull request as draft March 9, 2024 06:41
@qryxip qryxip removed the request for review from Hiroshiba March 9, 2024 06:41
@qryxip qryxip marked this pull request as ready for review March 9, 2024 09:30
@qryxip
Copy link
Member Author

qryxip commented Mar 9, 2024 •
edited
Loading

InferenceDomainSet InferenceDomainGroupという概念を導入し、一つのStatusが複数のInferenceDomainを扱うようにしました。

@qryxip qryxip requested a review from Hiroshiba March 9, 2024 09:35
@qryxip qryxip removed the request for review from Hiroshiba March 10, 2024 03:46
@qryxip qryxip marked this pull request as draft March 10, 2024 03:46
qryxip
qryxip commented Apr 14, 2024
View reviewed changes
crates/voicevox_core/src/infer/status.rs
Copy link
Member Author

@qryxip qryxip Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

差分が多すぎて移動扱いされなくなってしまった。とりあえず私は

❯ diff -u <(git show main:crates/voicevox_core/src/infer/status.rs) crates/voicevox_core/src/status.rs | delta -s

のようにして確認しています。
(delta(1)はMSYSにもあります: mingw-w64-x86_64-delta)
(コードエディタの拡張 (e.g. VSCodeのGitLens)にもしかしたらそういう機能があるかもしれない)

crates/voicevox_core/src/status.rs Outdated

impl InferenceDomainMap<[bool]> {
fn accepts(&self, style_type: StyleType) -> bool {
let InferenceDomainMap { talk } = *self;
Copy link
Member Author

Choose a reason for hiding this comment

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

#761 (comment)

@qryxip qryxip marked this pull request as draft April 14, 2024 08:25
@qryxip qryxip marked this pull request as ready for review April 14, 2024 08:25
Hiroshiba
Hiroshiba reviewed May 4, 2024
View reviewed changes
crates/voicevox_core/src/infer/session_set.rs Outdated
InferenceSessionOptions, InferenceSignature, ParamInfo,
};

pub(crate) struct SessionSet<R: InferenceRuntime, D: InferenceDomain>(
Copy link
Member

Choose a reason for hiding this comment

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

このSessionが何を指すか、初見だとわからないかもと思いました！
僕たちはonnxruntime.Sessionで見慣れてますが、なにか違うものだと勘違いするかも。

例えばInferenceSessionSetにするのはどうでしょう。SessionCellも同じく。
あとSessionとは何なのかの一言ドキュメントがあれば追いやすいのかなと思いました。まあ･･･説明難しいのですが･･･。

Copy link
Member Author

Choose a reason for hiding this comment

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

これまではstatus.rsの中だけで使ってたので名前を簡潔にしていたのですが、pub(crates)にするなら"Inference"は付けた方が確かにいいですね。
734564f (#761)

"session"については語感でなんとなく把握してもらえないかなと思っています。とはいってもTFLiteとかに手を出すときは整理をつける必要がありますし、infer/下の諸々と一緒にドキュメントは整備したい感はあります。

Copy link
Member

Choose a reason for hiding this comment

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

たしかにinfer以下にあれば察しはつくかもですね。

あーでも１回の推論ごとに１回セッションができると勘違いしてコード読み進めちゃう可能性はありそう。
モデル（Operation？）と対応する概念だという説明はまあなくても読んでればわかるけどあった方が良さそう。

Copy link
Member Author

Choose a reason for hiding this comment

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

まああった方はよいですね。TODOを残しました。
275d3d4 (#761)

Hiroshiba
Hiroshiba reviewed May 4, 2024
View reviewed changes
crates/voicevox_core_python_api/python/voicevox_core/_models.py Outdated
class StyleType(str, Enum):
"""**スタイル** (_style_)に対応するモデルの種類。"""

TALK = "TALK"
Copy link
Member

@Hiroshiba Hiroshiba May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

あ。小文字の snake_case でどうでしょう。
深い意味はないのですが、pythonエンジンがそう作っちゃったので合わせとこうかなと。

あと大文字が続くと英語勢には読みにくいらしいという言い訳もあります。
SINGING_TEACHERとか読みづらいかもしれない。

Copy link
Member Author

@qryxip qryxip May 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

本体(Rust API)だとsnake_caseなので、Python/Java API側のバグですね。Python APIだとvoice_model.metasとかsynthesizer.metasで死ぬ状態になってました。テストが無かったのでpytestを追加しておきました。
43fa77d (#761)

Javaの方はmetasに対するテストがあったのですが、SCREAMING_SNAKE_CASEのままでも通ってたようです。確認してはいないのですが、GSONってデフォルトでcase insensitiveでデシリアライズするのかな…?

Copy link
Member Author

Choose a reason for hiding this comment

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

Javaの方はmetasに対するテストがあったのですが、SCREAMING_SNAKE_CASEのままでも通ってたようです。確認してはいないのですが、GSONってデフォルトでcase insensitiveでデシリアライズするのかな…?

確認してみたところ、SCREAMING_SNAKE_CASEだとtypeがnullになってました。case sensitiveではあるようです。失念していたのですがGSONはenumに限らずJSON中の読めなかった部分を全部サイレントにnullとかにするので、何とかしたいですね。何とかできないのなら…まあテストを書く?

Hiroshiba
Hiroshiba approved these changes May 4, 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！！

いろいろコメントしましたが、こちらのPRでクリティカルに問題になりそうなとこは思いつかなかったです！
実装ありがとうございました！！！！！！

どのstructがどういう役割を持っているのか、設計を見失っちゃいそうなりますね！
Synthesizerとかstatusとか。（どちらも１つのインスタンスから全推論ドメインの情報を見れる）

sing APIを実装すると役割がわかりやすくなりそうなので楽しみです。

approveなので特に再レビュー不要そうだったらマージしていただければ！

@qryxip
Copy link
Member Author

qryxip commented May 5, 2024

レビュー対応しました。

Hiroshiba
Hiroshiba approved these changes May 6, 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！！

チェックをVoiceModelに移動したのなるほどでした。こっちのほうが良い気がしました！

@Hiroshiba Hiroshiba merged commit e55ea58 into VOICEVOX:main May 6, 2024
38 checks passed
@qryxip qryxip mentioned this pull request May 11, 2024
Open
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