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

追加: パッケージ内 run.py とその実行を追加 #1376

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

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 1, 2024

内容

概要: パッケージ内 run.py とその実行を追加

VOICEVOX ENGINE は起動用スクリプトとして run.py を定義している。
これはプロジェクトルート直下に置かれた Python スクリプトであり、voicevox_engine Python package とは独立した存在である。形式的には、外部の run.py スクリプトが voicevox_engine モジュールを呼び出す形になっている。

しかし、run.py は VOICEVOX ENGINE のサーバー定義をするなど、本質的に voicevox_engine package の一部である。独立した呼び出し用スクリプトではない。
run.py の正しい立ち位置は「voicevox_engine 内部のエントリーポイントモジュール」である。
であれば、__main__.py の仕組みを使ったエントリーポイント化が正しい実装である。
これに伴い、実行コマンドの意味が「起動用スクリプト run.py を叩く」から「voicevox_engine モジュールを実行する」へ変更される。

ただし、run.py は PyInstaller によるビルドの起点にもなっており、ビルドへの配慮が必要である。
昨今のリファクタリングにより run.py 自体はファイル位置に依存しなくなっている(例: パス取得)。ゆえに配慮すべきは datas 等の非 Python 依存性のみである。
幸い、これは run.spec の調整のみで実現できる。

このような背景から、パッケージ内へ run.py を移動し実行コマンドを変更することを提案します。

以下の動作が確認済みです:

  • コマンドからの VOICEVOX ENGINE 実行(python -m voicevox_engine --enable_mock
  • ビルド
  • ビルド産物の実行と API HTTP 呼び出し
  • --voicevox_dir オプション付き実行と API HTTP 呼び出し

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner June 1, 2024 05:55
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 1, 2024 05:55
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 3, 2024

うーーーーーーーーーーーーーん

なしでは無いのですが、移動できるから移動してみた、という感じがちょっとしました!
いくつか引っかかりを列挙してみます。

  • 初コントリビューターが入口を見つけられない
    • python -m voicevox_engineが何をやってるかわからなそう
      • 比べるとpython run.pyはなんとなくわかりそう
    • まあ、そもそも見つけづらかった気もするので、コードの概要があると大丈夫そう
    • とりあえず貢献者ガイドラインに1行だけでも入口の紹介があれば、まあ。
  • run.pyはともかくrun.specvoicevox_engineパッケージなのか
    • CUDAの情報とかもあるので、まあ流石に違う
    • run.specファイルの一番上のドキュメント案内で、これはビルド用のやつだと書いていれば、まあ。。。。。。
    • でもネガティブな要因だと思う、どうにか排除できると良さそう感
    • build_utilに入れるのがあってそうだけど、ディレクトリ名がscriptsに変わる場合また迷子になりそう
      • tools・・・?

プルリクの段階だと、利点と欠点が拮抗してる(ほんの少し利点のが大きい)という印象です!
run.specがきれいに片付いたら、あとは将来にコード概要を書けばOKな感じがしました!

@Hiroshiba
Copy link
Member

あ。 もしよかったら他の方の意見も聞いてみたいです 🙏
そんなにリポジトリ構成に詳しくないので・・・。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 3, 2024

python run.pyはなんとなくわかりそう

私の経験は逆でした。
最初期にコントリビュートしようとしたとき、パッケージ外の(ビルドスクリプトなのか何なのかよくわからない)run.py が主要機能を果たしているとは思わず、それなりに迷走しました。

python -m voicevox_engineが何をやってるかわからなそう

Python 開発者にとってはむしろ明確です。
__main__.py は Python の仕様なので、迷いなくこのファイルを見に行きます。

そもそも見つけづらかった気もするので、コードの概要があると大丈夫そう

👍️
同意です。
VOICEVOX ENGINE に限らず、現在の活発な OSS はどれもエントリーポイントが探しづらいです。多分プログラミングの規模がパッと認識できるサイズ感を超えているんだと思います。
なのでコード概要が良い解決策に感じます。

run.specvoicevox_engineパッケージなのか

👍️
妥当な指摘です。
少なくとも voicevox_engine から移動する必要を感じます。
前提として動かせるのかよくわからないので、検証してみます。
追記: voicevox_engine/run.specbuild_util/run.spec へ移動し、動作を確認しました。commit します。

@sabonerune
Copy link
Contributor

あまり詳しくないですが__main__.pypypi経由でインストールするアプリケーションというイメージがあります。
なので参考にするべきはpypiで公開されないようなアプリケーションだと思うのですがPythonで作られたアプリケーションというものをほぼ知りません。

あともう一つ注意する問題としてPyInstaller__main__.pyの相対パスを正しく処理できない問題があることです。
この問題は一応__main__.pyで常に絶対インポートを使用することで回避できるようですが。
pyinstaller/pyinstaller#2560 (comment)

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.

run.specの移動はともかく、run.pyの移動をすべきなのかとても迷ってます。

まずコミッターにとってよく使う起動コマンドの変更は破壊的変更なので、気軽にぽんぽん変えたくない気持ちがあります。
コミッター全員が変更に対応しないといけない。
変えても良いけど、こっちのが良い理由がほしいです。
「普通pythonアプリはpython -m起動だよ」という有名な前例があれば良さそう。
(pypiやpipなど、そのプロジェクトが依存する何かを起動するときはpython -mで良さそう)

あとrun.pyでやってることがvoicevox_engineパッケージ内でやる範疇を超えている気がしないでもないです。
resourcesディレクトリを見に行きますが、これはvoicevox_engineパッケージの責務というより、起動スクリプトの責務な気がします。
将来的にscripts/run.pyに移る可能性があるかもとか、本当にこれが最高なのか考えておきたい気持ちがあります。

あとpython -mで起動する方法は、まあpythonそこまで知らない人にとって何が起こるのかわからないと思います。
なのでコードの入口がどこなのかの案内は必須だと思います。
案内とセットじゃないと貢献しづらくなるという判断です。
(逆にこの問題があるからpython hoge.py起動のが良いのではと感じています)
まあこれはpython voicevox_engine/run.pyにすればOKな気がします。

それとPRタイトルと内容が変わってきてるかもです。これはタイトル変えれば良さそう。

ごたごた言ってすみません 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

run.spec 現状

#1408 にて root 直下ファイルの大規模移動は一旦見送りとなりました。これに伴い run.spec の移動は NoGo となりました。

run.py 現状

root の散らかりが現状維持となった結果、エントリーポイントを案内する必要性が上がりました。これを受けて「run.py 移動 + 丁寧なエントリーポイント案内」との方向性が示されています (ref)。

また run.py の更なるリファクタリングも進行しており(例: #1401, #1402)、run.py の責務がより明確化されそうです。

よって PR open のままレビューを一旦停止し、整理が落ち着いた段階で Go/NoGo の議論を再開できればと考えています。

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.

3 participants