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

整理: app 起動に依存せずユーザー辞書を立ち上げ #1231

Merged
merged 3 commits into from
May 17, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 11, 2024

内容

概要: app 起動に依存せずユーザー辞書を立ち上げるようリファクタリング

現在の VOICEVOX ENGINE はユーザー辞書の初期化を FastAPI app の立ち上げ hook (lifespan) 内でおこなっており、そのために hook 用の lifespan 関数を定義している。
立ち上げ hook は uvicorn でのサーバー起動時に動く(と思われる)が、ユーザー辞書初期化はそれより前におこなわれても問題はない。
ゆえに必要のない hook 待機のためにコードが複雑化しているといえる。
app インスタンス生成前に独立してユーザー辞書初期化をおこなえば、この hook 用コードを削除できる。

このような背景から、app 起動に依存せずユーザー辞書を立ち上げるリファクタリングを提案します。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner May 11, 2024 13:36
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 11, 2024 13:36
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!!

起動時間が伸びるかもなのが少し気がかりでした!
辞書登録数が数百とかだとまあまあかかったり…?

その場合は辞書初期化を遅延させるてもあるかもと感じました!
initializeしたかをUserDictクラスで持っとく、みたいな。

@Hiroshiba
Copy link
Member

あっコンフリクトしてますね…🙇

@tarepan
Copy link
Contributor Author

tarepan commented May 14, 2024

起動時間が伸びるかも

  • 変更前: generate_app()uvicorn.run()lifespan() → API 受付可能(=起動)
  • 変更後: generate_app()lifespan() 等価 → uvicorn.run() → API 受付可能(=起動)

となっているため、エンジン起動時間は本 PR から影響受けないと考えています。

辞書登録数が数百とかだとまあまあかかったり
...
その場合は辞書初期化を遅延

👍️
妥当な指摘です。
大事にしたい長期ユーザーほど影響を受けうる箇所なので重要な視点だと思います。
実装を今するかは別として、ベンチマーク測って注視しておく必要がありそうです。


コンフリクト解消しました。merge 可能です。

@Hiroshiba
Copy link
Member

あっそうですね、起動時間を伸びなさそう!!

コンフリクト解消ありがとうございます!!

@Hiroshiba Hiroshiba merged commit fce90e7 into VOICEVOX:master May 17, 2024
4 checks passed
@tarepan tarepan deleted the refactor/first_update_dict branch May 17, 2024 17:21
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