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

追加: bump@2024-05 #1246

Merged
merged 15 commits into from
May 17, 2024
Merged

追加: bump@2024-05 #1246

merged 15 commits into from
May 17, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 15, 2024

内容

各種パッケージのバージョンアップ

FastAPI は 0.110.0 以前の fastapi が 0.111.0 以降の fastapi-slim へと変更になっている。

test / lint / build / run が正常動作することをローカルで確認済み。

関連 Issue

resolve #1245

@tarepan tarepan requested a review from a team as a code owner May 15, 2024 06:53
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 15, 2024 06:53
@sabonerune
Copy link
Contributor

エンジンをCtrl + cで止めるとKeyboardInterruptが発生します。
どうやらUvicorn 0.29でシグナルの処理が修正されたらしいです。

コードとPRを軽く見た感じ、以前はSIGINTを受け取るとuvicorn.run()の処理がそのまま終了するという挙動でしたが0.29からUvicorn終了後にUvicornが処理したシグナルを再度送出し直すようになったようです。

-    uvicorn.run(app, host=args.host, port=args.port)
+    try:
+        uvicorn.run(app, host=args.host, port=args.port)
+    except KeyboardInterrupt:
+        pass

でとりあえず黙らせることはできますがこれが正しい方法かはまだ調べがついていません。
(もしくはシグナルハンドラを登録しておく?)

@tarepan
Copy link
Contributor Author

tarepan commented May 15, 2024

見落としていました、ありがとうございます!

VOICEVOX ENGINE は「ctrl + c でサーバーを正常終了する」という仕様を策定していないため、KeyboardInterrupt で落ちるのがむしろ正しい挙動な気もします。
逆に、暗示的仕様として正常終了を今まで想定してきた、という解釈もありえそうです。

取りうる選択肢は以下の 4 つと考えます:

  • ctrl + c の正常系を新たに明示定義する
    • 「終了コマンド」と定義し、catch して終了ログ流して close を新規実装
    • 「強制介入コマンド」と定義し、KeyboardInterrupt を投げっぱなし
  • ctrl + c の正常系を明示的には定義しない
    • 「今までの動作を維持」として catch を実装
    • 「実装依存」として KeyboardInterrupt を投げっぱなし

ここはメンテナ判断になりそうです(@Hiroshiba)。

@Hiroshiba
Copy link
Member

Hiroshiba commented May 15, 2024

なるほどです、気づいてくださってありがとうございます!!

エディタ側からはsigintで停止してる感じでしたっけ👀
もしそうなら、仕様として今後サポートする方針が良さそうに感じました!
そうじゃなければ別にエラーでも構わないかもとか思いました!

方針は放置して何もしないか、仕様にして実装が良いかなと。

気が向けば明示的な仕様にしても良いかも。そういえば停止方法がなかったので。
…サーバー停止って他のプロダクトはどうやって受け付けてるんだろ。jupyterとか。

自信ないので、小さくてもOKなので課題点思いつかれた場合はぜひ知りたいです🙇

@sabonerune
Copy link
Contributor

あまりsignalについて詳しくないのですが…

エディタ側からはsigintで停止してる感じでしたっけ👀

https://github.com/VOICEVOX/voicevox/blob/4c9550b089aeda07153a2ba203c54d27b988e9ff/src/backend/electron/manager/engineManager.ts#L504
Windowsではtaskkillコマンド(恐らく強制終了?)で停止されます。
https://github.com/pkrumins/node-tree-kill/blob/cb478381547107f5c53362668533f634beff7e6e/index.js#L29
POSIXでは全てのプロセスに対してSIGTERMを送ることで停止しているはずです。
https://github.com/pkrumins/node-tree-kill/blob/cb478381547107f5c53362668533f634beff7e6e/index.js#L82
https://nodejs.org/docs/latest-v20.x/api/process.html#processkillpid-signal

なおWindowsのsignalはPOSIXとかなり異なるらしくNodeの機能からCtrl+CやCtrl+Breakを出すことができないようです。

そうじゃなければ別にエラーでも構わないかもとか思いました!
方針は放置して何もしないか、仕様にして実装が良いかなと。

Ctrl+C押してエラーが出るのは個人的には気になる…
あとINFO: Uvicorn running on http://localhost:50021 (Press CTRL+C to quit)とコンソールに出ているのですよね…
「何もしない」という動作をさせるのはUvicornの動作を変えるためのコードが必要でかなり面倒だと思います。

@tarepan
Copy link
Contributor Author

tarepan commented May 15, 2024

シグナルハンドリング & シャットダウンは丁寧な議論が必要そうなので issue 化しました(#1248)。

本 PR はリファクタリング的なライブラリアップデートであるため、挙動変更無しが理想的です。
よって提案頂いた except KeyboardInterrupt で対応します。

@tarepan
Copy link
Contributor Author

tarepan commented May 15, 2024

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

@sabonerune
もしお時間ありましたらレビュー頂けると心強いです(fastapi-slim の話題を追っていらしたようなので)。

@sabonerune
Copy link
Contributor

pysenの更新でflake8の設定が変わったようです。
pysen generate .で更新をお願いします。

後は多分大丈夫だと思います。

@tarepan
Copy link
Contributor Author

tarepan commented May 16, 2024

pysenの更新でflake8の設定が変わった

👍️
見落としていました、指摘ありがとうございます!
pysen=0.11.0 で linter 間コンフリクトの解決があり、これで setup.cfg に更新が入っていました。提示頂いたコマンドで setup.cfg を正常に更新できました。

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

更新ありがとうございます!!

@Hiroshiba
Copy link
Member

@sabonerune さん的にも大丈夫そうだったので、たぶん大丈夫ということでマージしちゃいます!
なにかあればコメントいただけると心強いです 🙇

@Hiroshiba Hiroshiba merged commit 9b2944e into VOICEVOX:master May 17, 2024
4 checks passed
@sabonerune
Copy link
Contributor

問題ないと思います

@tarepan tarepan deleted the add/bump202405 branch May 18, 2024 03:00
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.

add: パッケージ bump 2024-05
3 participants