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

add: .get_engine() で latest version を取得できるように #1421

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 23, 2024

内容

#1391 に従い、.get_engine(None) による最新版 TTSEngine の取得を追加するリファクタリングを提案します。

本 PR により routers のコア依存が大幅に減少します。

関連 Issue

part of #1391

@tarepan tarepan marked this pull request as ready for review June 27, 2024 05:49
@tarepan tarepan requested a review from a team as a code owner June 27, 2024 05:49
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 27, 2024 05:49
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.

tts_enginesにget_latest_version関数を実装すれば元の実装に近い形でいける気がします。

もしかしたらNoneをそのまま使う形にする提案を別の形で再度提案、ということかもなのですが、いろいろ実装してから再考するのはどうでしょうか。
(強い意見でしたら今再考します!)

@tarepan
Copy link
Contributor Author

tarepan commented Jun 27, 2024

tts_enginesにget_latest_version関数を実装すれば元の実装に近い形でいける気がします。

以下のような形で認識あっているでしょうか?

def api_x(core_version: str | SkipJsonSchema[None] = None):
    version = core_version or tts_engines.latest_version()
    engine = tts_engines.get_engine(version)

こちらの実装をする場合、命名についてご相談したい部分があります。

version = core_version or tts_engines.latest_version() の部分、新規コントリビュータが見ると version = core_version or latest_tts_version という意味に見えると思います。感じ方としては「コアバージョンとTTSバージョンはどういう関係なんだ…?そのあと get_engine() に渡してるけど…」となりそうです。
これを避けるなら tts_engines.latest_core_version() とする手がありそうです。こうすると version = core_version or latest_core_version となり、曖昧さが無くなります。しかし同時に、内蔵して隠蔽したはずの「コア」に関する知識が TTSEngineManager から漏れ出てきている感じがします。

次の 2 点についてお聞きできれば幸いです:

  • core_version or tts_engines.latest_version() に違和感があるか否か
  • tts_engines.latest_core_version() にコア漏れを感じるか否か

Noneをそのまま使う形にする提案を別の形で再度提案

あんまり意識していなかったんですが、確かに #1311 / #1317 あたりの議論ですね。
コア内蔵の方針がなかったため core_manager.latest_version() が可能であり当時は違和感無かったんですが、tts から取るとなんか違和感が拭えない感あります。

強い意見でしたら今再考します

アーキテクチャに関わる部分ではないので、わりと緩い意見です。すわりが悪い的な、なんか違和感あるといったニュアンスです。
@Hiroshiba さんが(上記質問にある)違和感が弱めであればサクッと or tts_engines.latest_version() へ実装変更します。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 27, 2024

なーーーるほどです!!

core_version or tts_engines.latest_version() に違和感があるか否か
tts_engines.latest_core_version() にコア漏れを感じるか否か

どちらも同感です(違和感ある・漏れてる)。
後者はtts_engines.get_engine(core_version)でも漏れを感じるかもです。

・・・これは本質的に漏れざるを得ない気がしますね!!
managerはコアのバージョン情報を管理してるので、把握していても不思議じゃなさそう。

漏れは感じるけど、tts_engines.latest_core_version()が良さそう、という感じで・・・!!

コア内蔵の方針がなかったため core_manager.latest_version() が可能であり当時は違和感無かったんですが、tts から取るとなんか違和感が拭えない感あります。

なるほどです、大変失礼しました。。 🙇

そして仰るとおりな気がしてきました。
違和感を言語化すると「Noneはlatestであるというルールがいたるところに貫通している」のが気になっているというだけかもです。
となると、Noneじゃなく何かしらのリテラル値、例えば"latest"にすれば一番問題ないかもです。
文字列"latest"だとバージョンstringと型が揃ってしまいますが・・・。なにかうまい方法があれば。。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 27, 2024

「Noneはlatestであるというルールがいたるところに貫通している」のが気になっている ...
例えば"latest"にすれば一番問題ないかも

👍️
None に意味を持たせて流通させるとバグりがち(不意に None 代入したり)なので、真っ当な直感だと思います。
提案ありがとうございます、こちらの方法で実装試してみます (NewType あたりが使えそう?) 。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 27, 2024

Literal, TypeAlias, Constant (Final) を利用して NoneLATEST_VERSION の置き換えと LATEST_VERSION チェックを導入しました。LATEST_VERSION は厳密な固定値(変更・再代入が不可)になっているため、この値を version へ代入し version がこの値かチェックすることで最新版指定か否かを確認できるようにしました。

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

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

リテラル文字列型にしてから値代入するの、ちょっと面倒だけどstringと分けれるの良いですね!!
タイトルだけ変えさせていただきます!

Comment on lines -248 to +250
if tts_engines.has_engine(version):
try:
_engine = tts_engines.get_engine(version)
else:
except Exception:
Copy link
Member

@Hiroshiba Hiroshiba Jun 28, 2024

Choose a reason for hiding this comment

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

!!

has_engine危ないかも?(LagestVersionが受け付けられない・・・けど型的には大丈夫か。)

確認したらここでの利用がなくなって、どうやらhas_engineがテストからしか使われなくなったっぽみです。
一緒に消してしまっても良いかも・・・?

だけどまあこのPRは問題じゃないから大丈夫そう!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

見逃していました、指摘ありがとうございます!
#1446 にて削除します。

@Hiroshiba Hiroshiba changed the title add: .get_engine() に None 入力による latest 取得を追加 add: .get_engine() で latest version を取得できるように Jun 28, 2024
@Hiroshiba Hiroshiba merged commit 000b363 into VOICEVOX:master Jun 28, 2024
4 checks passed
@tarepan tarepan deleted the refactor/get_engine_latest branch June 28, 2024 05:41
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