-
Notifications
You must be signed in to change notification settings - Fork 209
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
FIX: 非推奨のstartup
イベントをlifespan
に置き換え
#1131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM👍
機能を維持しつつ、新しい起動時イベントへ更新されています。good work!
本来は
lifespan
で使用するオブジェクト ... を初期化
ざっとドキュメント読んだ感じ、強制ではないけど推奨な感じかなと思いました。
uvicorn.run()
の前に初期化処理しなくて良くなるのが利点…ぽい?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
ざっとドキュメント読んだ感じ、強制ではないけど推奨な感じかなと思いました。
uvicorn.run() の前に初期化処理しなくて良くなるのが利点…ぽい?
単純にライフサイクルに乗っけとけば、将来FastAPIに機能が追加されてサーバーをスケーリングできるようになった時とかに便利なのかなぁとかは思いました。
現状に本当にどっちでもいい気がします!
個人的には、仮にコアの初期化とかに失敗するような環境だったとして、エンジンを起動しようとしてからライフサイクルが始まってエラーになるよりも、最初に用意しようとした時に用意できないことを知らせるようにしといた方がメンテナンスが高いような気がしました。
あと単純にstartup関数内で初期化すると、初期化する変数全部がOptionalになったりして不便そうな気がします!
内容
以下のコードが原因で
DeprecationWarning
が発生していたので更新します。voicevox_engine/run.py
Lines 251 to 253 in c75b657
関連 Issue
その他
動作上は全く変わらないと思います。
ref : Lifespan Events - FastAPI
ref : Lifespan - Starlette
ドキュメントを見て思ったのですが本来は
lifespan
で使用するオブジェクト(VOICEVOXではtts_engines
やcores
)を初期化することが想定されている?