-
Notifications
You must be signed in to change notification settings - Fork 204
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
BLD: PyInstallerをv6へ更新 #857
BLD: PyInstallerをv6へ更新 #857
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.
まだビルド回り読めてないのですが、コード周りだけレビューしてみました!
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.
ドキュメント追加ありがとうございます!!
1点ご存知でしたらお伺いしたいことが!
もし可能ならonnxruntime.dll
などの配置を以前と同じようにpyinstaller側に任せるのが比較的綺麗かなと思っています。
(実行に必要な依存関係をpyinstallerビルドスクリプト内に記述できるので)
internalに配置するファイルと、ルート直下(run.exeと同じディレクトリ)に配置するファイルを別々に指定する方法などはありそうでしょうか・・・?
自分が調べた限りspecファイル内でrun.exeと同じディレクトリにファイルを配置する方法は見つかりませんでした。 スクリプト一つで完結させるならbuild_utilにPyInstallerの呼び出しとルート直下へのファイル移動を行うコードを入れるくらいしか思い浮かびませんね… |
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.
色々調整してくださってありがとうございます!!
1個だけちょっとご相談があります。
今まで実行可能なバイナリをビルドするコマンドが1行だけで済んでいた(pyinstallerコマンド)のが、ビルドした後にファイルコピーコマンドを実行する必要がある状況になっていることに気づきました。
色々調べた感じ--add-binaryしたらDLLファイルは配置できそうでしたが、speaker_infoディレクトリなどは難しいかもしれません。
argparseで色々必要なファイルのパスを指定してdist
ディレクトリにコピーする、ということをspec内で行えば今まで通りpyinstallerコマンドのみでビルドできるかもなのですが、どうでしょうか・・・・?
.github/workflows/build.yml
Outdated
run: | | ||
set -eux | ||
|
||
# Move DLL dependencies (cache already saved) | ||
cp download/core/libvoicevox_core.so dist/run/ | ||
rm -rf download/core |
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.
これちょっと罠なのですが、多分このディレクトリはキャッシュしてるので消したらダメかもです。
正確には消しても別に問題ないのですが、確か全ての定義済みのステップが完了した後にそのディレクトリをキャッシュするという仕様だった気がします。
ディレクトリを消しているとそこでキャッシュがされないので、またダウンロードが走る感じになるかなと!
(ぶっちゃけもう1回ダウンロードしてもあまりビルド時間は変わらないのですが、まあ気持ち的に・・・!)
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.
元々PyInstaller側でコピーしていたものをspecファイルで処理するようにしたのでほぼ元のコードに戻りました。
後続のライブラリの処理で削除していたのでそれに倣った感じです。
確かキャッシュは原則的に明示的に(またはActionによって間接的に)保存処理をする必要があったはずです。
このworkflowはファイルをダウンロードして展開した直後にキャッシュ保存をしていたので問題ないはずです。
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.
このworkflowはファイルをダウンロードして展開した直後にキャッシュ保存をしていたので問題ないはずです。
確認してみたのですが、onnxruntimeなどはおっしゃる通りの実装になっていると思います(↓のとこで保存している)!
voicevox_engine/.github/workflows/build.yml
Lines 381 to 385 in 6a2a010
- name: Save ONNX Runtime cache | |
uses: actions/cache/save@v3 | |
with: | |
key: ${{ steps.onnxruntime-cache-restore.outputs.cache-primary-key }} | |
path: download/onnxruntime |
でもコアとRESOURCEだけ(?)はその形になっていなさそうだなと・・・!
(どちらかと言うと、この2つも明示的にsaveを呼ぶのが良い気がしています)
specファイル内で実行ファイルのディレクトリにファイルを移動するオプションがなかったのでできないものと思っていましたがどうやらできるようです。 |
@sabonerune 検討ありがとうございます、是非!!! |
specファイル内での処理するように変更しました |
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です!!
細かい部分で開発しやすさをそのままにしておいてあげると嬉しそうかもなのでコメントしてみました 🙇
とりあえずリポジトリにデフォルトで存在しないファイルはなくてもビルドが通るようにしました。 |
ありがとうございます、すごく良さそうに思います!! |
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!!!!
確認としてプレビュー版リリース0.15.0-preview.10
を作りたいと思います!
ありがとうございました!!!
ビルド周りはまだまだ改善の余地がかなり残っていると思います。
もしご興味あればジャカジャカ進めていただけるとすごく助かります。もっと気軽にissueなどでコメントなどいただけると!!
This reverts commit 4ef4218.
内容
PyInstaller v6へ更新します。
それに伴いビルド後にPyInstallerが利用するファイルが全て一つのディレクトリに移動されたため
engine_root()
周りの処理、ビルドやCIの変更があります。関連 Issue
その他
PyInstaller内部で利用するディレクトリ名は
engine_internal
にしました。