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

change: VoiceModelVoiceModelFile #832

Merged
merged 19 commits into from
Sep 19, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Sep 15, 2024

内容

#829 を行います。変更は大体こんな感じです。

変更前 変更後
音声モデル。 音声モデルファイル。
VoiceModel (C以外) VoiceModelFile (C以外)
VoicevoxVoiceModel (C) VoicevoxVoiceModelFile (C)
変更前 変更後
VVMファイルからVoiceModelをコンストラクトする。 VVMファイルを開く。
VoiceModel::from_path (Rust, Python) VoiceModelFile::open (Rust, Python)
voicevox_voice_model_new_from_path (C) voicevox_voice_model_file_open (C)
変更前 変更後
N/A VVMファイルを閉じる。
N/A VoiceModelFile::close (Rust, Python, Java)
N/A VoiceModelFile.__a{enter,exit}__ (Python, Java)
voicevox_voice_model_delete (C) voicevox_voice_model_file_close (C)

Synthesizer::load_voice_model中にVoiceModelFile::closeしたときの挙動はこんな感じになっているはずです。ただしまだこれらについてのテストは書いてませんし手動での確認もやってません

言語 Synthesizer::load_voice_model中にcloseしたときの挙動
Rust (同期版 & 非同期版) そもそも起こり得ない
Python (同期版 & 非同期版) 例外(Exception)を発する Javaと同様にロックして待つ
Java load_voice_modelが終わるまでロックして待つ
C (ユーザーに課してる規約上)そもそも起こり得ない

[追記1] Python APIでload_voice_modelが終わるまでロックして待つのが面倒そうに感じたのでこうなってしまったのですが、よく考えたらそんなに難しくなかった。それよりもロックするようなデザインにするか例外を発するようなデザインにするかですね。

[追記2] ロックする方のデザインの方がいいかもですね。reader-writer lockが一つあれば実装できますし、C APIにもAPIをシンプルにしたまま仕込める。

[追記3] Python APIをロックするデザインにしてしまいました。こんな感じになります。

    async with await VoiceModelFile.open(vvm_path) as model:
        _ = synthesizer.load_voice_model(model) # awaitしない
[WARNING] voicevox_core_python_api: The `VoiceModelFile` is still in use. Waiting before closing
[DEBUG] voicevox_core_python_api: Closing a VoiceModelFile
# load_voice_modelが無事完了している

関連 Issue

Resolves #829.

その他

@qryxip qryxip force-pushed the change-voice-model-to-voice-model-file branch from 0432206 to f32872e Compare September 16, 2024 04:30
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.

なるほどな設計に思いました!!

すみません、インターフェイスを見て今更思いついたのですが、これってc api等で2つ以上のSynthesizerにopen済みのModelFileを登録(ロード?)した場合に問題などは生じそうでしょうか?
(どうするかはさておき、一旦問題が起きそうかだけお聞きしてる感じです!)

@qryxip
Copy link
Member Author

qryxip commented Sep 17, 2024

現行実装と変わらず、特に問題無く並行実行できるはずです。close時に1箇所以上からのアクセスがまだ走ってるなら、そのすべてが終わるまでロックするという感じです。私がreader writer lockと言ったやつについては(呼び方は違いますが)以下リンクで解説されています。
共有/排他ミューテックス(Shared-Exclusive Mutex) - マルチスレッド・プログラミングの道具箱 - Zenn

(まあ並行実行のテストは無いです。が、並行実行のテストというのも難しいのでこういう感じのベンチのコードを管理して手軽に確認できるようにするという感じがよさそう)
#746 (comment)

@Hiroshiba
Copy link
Member

おおなるほどです!!問題なさそう!
レビュー始めたいと思います🙏

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なのですが、Python版に関してちょっと気になることがあったのでそこだけという感じです!

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

qryxip commented Sep 18, 2024

Copy link
Member Author

@qryxip qryxip Sep 18, 2024

Choose a reason for hiding this comment

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

このコードをそのままChatGPT 4oに投げ付けてみたら一発で満点の答えを返してくれた。こういうところではコメントでの説明を省いていきたいなーと個人的には思っている。
https://chatgpt.com/share/66ea8dce-a580-800c-b4fd-37af1084bfa5

Copy link
Member Author

@qryxip qryxip Sep 18, 2024

Choose a reason for hiding this comment

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

実装の中身については、G: 'a, T: 'static (∀.'a)という制約を入れることで"T ourlives G"の関係にしてouroborosに突っ込めばSafe Rustで記述しきれる、というのが肝。

ただChatGPTがこのことに辿り着くのは難しそう。「この'aは?」って聞いてもわかってなさげな答えを返してきた。

…やっぱコメント書くべきか?いやRust詳しい人ならわかるだろうし…

Copy link
Member Author

@qryxip qryxip Sep 18, 2024

Choose a reason for hiding this comment

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

11be136 (#832)

T: 'staticについてwhyのコメントが要りそうだったので、今回の場合はdocstringに入れることにしました。

Copy link
Member Author

@qryxip qryxip Sep 18, 2024

Choose a reason for hiding this comment

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

あ!!! T outlives Gの関係にするのにT: 'staticにする必要は無い!

ということで、whyのドキュメントは消えてまたコメント無しに…
74a482d (#832)

追記: さらに改修して、今はこんな感じになりました。
https://github.com/qryxip/voicevox_core/blob/aed651b9105046b37036f0178368abdf15b96f64/crates/voicevox_core/src/__internal/interop/raii.rs#L10-L43

Copy link
Member

Choose a reason for hiding this comment

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

ここについて色々考えてみて結論としては

  • Rust初学者には相当難しい
  • とはいえここはユーティリティで、ここを変更しに来る人はものすごい珍しい
  • なので、求めてるものはここじゃないことがわかればいい
  • try_map_guardという名前はwhatもわからないけど、処理を大きく変えてくるような印象はない
    • つまりスルーしても良さそうには感じる
  • 総合すると、今の形で OK !

という感じかなと!!

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 qryxip merged commit 967570a into VOICEVOX:main Sep 19, 2024
37 checks passed
@qryxip qryxip deleted the change-voice-model-to-voice-model-file branch September 19, 2024 00:48
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.

VoiceModelVoiceModelFileにし、ファイルディスクリプタを保持する
2 participants