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

整理: コア・エンジンでバージョンを指定しない場合、暗黙的に最新版を取得する処理を削除 #1317

Merged
merged 18 commits into from
Jun 18, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 24, 2024

内容

get_core() / get_engine() における最新版暗示的取得を削除するリファクタリングを提案します。

CoreManager.convert_version_format() を新設し、core_version=None を明示的に変換します。
get_core() / get_engine() はこの明示化されたバージョンのみを受け取ります。
このリファクタリングの結果、TTSEngineManager.latest_version() が不要となったため削除します。

関連 Issue

ref #1234 (comment)
ref #1227 (comment)

tarepan added 2 commits May 24, 2024 05:48

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@tarepan tarepan requested a review from a team as a code owner May 24, 2024 05:56
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 24, 2024 05:56
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.

結論出てないのですがとりあえずコメントまで 🙇

voicevox_engine/core/core_initializer.py Outdated Show resolved Hide resolved
voicevox_engine/core/core_initializer.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented May 27, 2024

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

なのですがいろいろコメント書いたので一旦お待ちします 🙇

FastAPIの機能でNoneをlatestに置き換えるとかできないかな。。

voicevox_engine/app/routers/commons.py Outdated Show resolved Hide resolved
voicevox_engine/app/routers/commons.py Outdated Show resolved Hide resolved
voicevox_engine/app/routers/commons.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Jun 3, 2024

Fasly 化で簡略化しました。
その結果、指摘のあったコードは丸々削除となりました。

FastAPIの機能でNoneをlatestに置き換えるとかできないかな。。

FastAPI で上手く抽象化しても、最終的には内部で動的に core_manager.latest_version() を呼ぶ必要があるので、しょうがない処理な気がします。


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

@Hiroshiba Hiroshiba changed the title 整理: 最新版コア・エンジンの暗示的取得を削除 整理: コア・エンジンでバージョンを指定しない場合、暗黙的に最新版を取得する処理を削除 Jun 18, 2024
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!!

タイトル変えさせていただきました。

最終的には内部で動的に core_manager.latest_version() を呼ぶ必要がある

あー・・・たしかにですね・・・。

@Hiroshiba Hiroshiba merged commit 95f218a into VOICEVOX:master Jun 18, 2024
4 checks passed
@tarepan tarepan deleted the refactor/explicit_core_version branch June 18, 2024 14:44
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