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

整理: エンジンが見つからない例外を追加 #1309

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 23, 2024

内容

概要: ドメイン内 HTTPException を新規の「エンジンが見つからない」例外に置き換えてリファクタリング

#1234 でエンジン取得がドメインへ移され、その際に HTTPException も変更されず移動された。これはサーバーとドメインのミスマッチであり修正すべきである。
新規例外を作成した場合、この例外をサーバー側のどこでキャッチするか(application.py か routers か)設計する必要がある。エンジンは複数の router で利用されているため、router でキャッチしようとすると各 router にキャッチ実装が必要で煩雑である。一方、application.py であれば FastAPI の global exception handler 機能を利用できる。よって後者が妥当である。

このような背景から、エンジン系 HTTPException を新規 EngineNotFound 例外に置き換えグローバルハンドラでキャッチするリファクタリングを提案します。

関連 Issue

ref #1234

@tarepan tarepan requested a review from a team as a code owner May 23, 2024 07:07
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 23, 2024 07:07
@tarepan
Copy link
Contributor Author

tarepan commented Jun 2, 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です!
ちょっと案内のとこだけ。。 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jun 3, 2024

「VOICEVOX "ENGINE"」と「TTS "Engine"」の混同が根本原因だとわかりました。

現在の EngineNotFound は「TTSEngine が見つからない」を意味しています。VOICEVOX ENGINE 内部的にはこれは正しい情報です。
一方で TTSEngine は内部実装でありユーザーには見えない情報です。ゆえにユーザー向けエラーメッセージで見せるべきではありません。
ここをあまり意識しなかった+読みやすくしようとした結果、「TTSEngine が見つからない」の意で「エンジンが見つかりません」とのメッセージになりました。結果としてこのメッセージは「VOICEVOX ENGINE が見つからない...????」との誤解を与える文章になっています。バージョン番号もユーザーからみるとわけわからないことになります。

本 PR を大幅に変更します。以下が変更の方針です:

  • EngineNotFoundTTSEngineNotFound にリネーム
  • TTSEngineNotFound の文字列メッセージでなく version field で情報伝達
  • exception hander で version 値を確認、モック番号でなければ「コアが見つからない」を API で返す

@tarepan
Copy link
Contributor Author

tarepan commented Jun 3, 2024

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

@tarepan
Copy link
Contributor Author

tarepan commented Jun 15, 2024

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re3-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!!

voicevox_engine/tts_pipeline/tts_engine.py Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あっ コンフリクト解消しようとしたのですが、LatestTTSEngineNotFoundが不要になってるかもです 🙇
あるいは別のところで発生するようになってる・・・?

ちょっとすみません、おまかせできると助かります 🙇 🙇 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

LatestTTSEngineNotFound が不要になったため削除しました。

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

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