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

整理: tts_engine モジュール内のプライベートな関数をリネーム、テストを整理 #1435

Merged
merged 22 commits into from
Jun 27, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 25, 2024

内容

概要: tts_engine モジュール内のプライベートな関数をリネームしてリファクタリング

tts_engine モジュールは TTSEngine 用のプライベートな関数を多数定義している。
これらは tts_engine モジュールに閉じて利用されているが、関数名に _ prefix が付いていないケースが散見される。これはコーディング規約 No.1 (#799)に反する。

このような背景から、tts_engine モジュール内のプライベートな関数をリネームするリファクタリングを提案します。

またリネームの過程でテストに改善点が見つかったため、同時にリファクタリングをおこなった。
主な変更点は以下の通り:

  • 主たる関数の出力と assert 用の変換を分離(_to_flatten_phonemes
  • 1 フレーム相当の時間を意味する定数を設定(T

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner June 25, 2024 17:29
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 25, 2024 17:29
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なのですが、ちょっと設計方針をお聞きしたいです!

tts_pipeline(というよりtts_engine?)は今後どうなっていきそうでしょうか・・・? 👀
それによっては今回の大量のプライベート関数化も方針が変わってきそうだなーと。

例えばsynthesis_engineになってモジュール化してファイル分割していく、とか。

test/unit/tts_pipeline/test_wave_synthesizer.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Jun 26, 2024

tts_pipeline(というよりtts_engine?)は今後どうなっていきそう

以下の変更を予定しています:

より長期の方向性としては「MockTTSEngine 無くせないか?」を検証中です(TTSEngine(NewMockCore) 化)。tts_engine.py の現行コードにはあまり影響しないと思われます。とても上手くいくと TTSEngineManager を無くして TTSEngine に全コアを載せる形が見えてくるので、それはそれで良いことなのか別の議論を立てることになりそうです。
どちらにしても tts_engine.py 分割的な方向性は今のところ考えていない感じです。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。
(追記: まだ push してなかった…)

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!

@Hiroshiba Hiroshiba merged commit 94c261c into VOICEVOX:master Jun 27, 2024
4 checks passed
@tarepan tarepan deleted the refactor/tts_engine_format branch June 27, 2024 03:51
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.

2 participants