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

Github Actions とpre-commitにpoetry exportを適用 #716

Merged
merged 5 commits into from
Jul 18, 2023

Conversation

FujisakiEx
Copy link
Contributor

内容

Github Actions とpre-commit のそれぞれで poetry export を行うように対応しました

関連 Issue

ref #566

スクリーンショット・動画など

image

その他

@FujisakiEx FujisakiEx requested a review from a team as a code owner July 9, 2023 10:27
@FujisakiEx FujisakiEx requested review from Hiroshiba and removed request for a team July 9, 2023 10:27
@FujisakiEx
Copy link
Contributor Author

FujisakiEx commented Jul 9, 2023

pre-commit.ci をGithubのリポジトリ側で有効にしてもらわないとダメなのを忘れていました
FujisakiEx#3
こっちで実行結果を確認できます

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 2 1 coverage-50%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 3 1 coverage-67%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/downloadable_library.py 93 5 coverage-95%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/metas/Metas.py 33 0 coverage-100%
voicevox_engine/metas/MetasStore.py 29 14 coverage-52%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 160 9 coverage-94%
voicevox_engine/mora_list.py 5 0 coverage-100%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetError.py 3 1 coverage-67%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 19 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 201 159 coverage-21%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 57 49 coverage-14%
voicevox_engine/synthesis_engine/synthesis_engine.py 130 11 coverage-92%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 144 11 coverage-92%
voicevox_engine/utility/init.py 5 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 9 2 coverage-78%
voicevox_engine/utility/mutex_utility.py 11 1 coverage-91%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
TOTAL 1516 289 coverage-81%

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.

プルリクエストありがとうございます!!
コードを書き足す箇所とかがかなり適切で、レビューがしやすかったです!!

.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 53 to 58
- name: Run poetry
run: |
poetry export --without-hashes -o requirements.txt
poetry export --without-hashes --with dev -o requirements-dev.txt
poetry export --without-hashes --with test -o requirements-test.txt
poetry export --without-hashes --with license -o requirements-license.txt
Copy link
Member

@Hiroshiba Hiroshiba Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このままだとrequirements.txtpoetry.tomlが乖離していた場合に勝手にrequirements.txtが上書きされちゃいそうです。(exportなので)
やりたいことは乖離していった時にエラーを出すことかなと!

たとえばrequirements.txt.checkにexportして、requirements.txtと差分があったらエラーにするのはどうでしょう?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントで挙げてもらった方法で対応しました

Comment on lines 16 to 9
- id: poetry-export
name: poetry-export
args: ["--without-hashes", "-o", "requirements.txt" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

実際にpoetry addしてpoetry exportしないままpushしてみたらちゃんとエラーが出ました!
ただエラーメッセージが若干不親切だなとちょっと思いました。

これ実際にエクスポートしているのでrequirements.txtファイルが置き換わるんですよね。
なので出来上がったファイルを再度コミットしてくださいというエラメッセージにするのはどうでしょう?
(そういうことってできそうでしょうか?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poetry_export_all.pyがpre-commitの際に実行されるように修正しました
このスクリプトはdiffに差分があった場合、メッセージを出すようになっています
キャプチャ

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほどです!!

ちょっと1点だけ、環境によってはpythonではなくpython3コマンドとしてPythonスクリプトを実行している人もいるはずで、そういう人にとってはエラーが出てしまうなと思いました!
どうにかできる解決策を調べてみたんですが、何か良い方法は見つかりませんでした・・・。
(シェルスクリプトを書くとかがあったのですが、これまたOSによっては動かないことがあるので)

特に初学者の方がpython3コマンドを使っていた場合、なぜエラーが出るのかわからずにちょっと戸惑ってしまうかもです。
poetry-exportがエラーになるのはpoetryを使えるぐらい熟練者の方なので、自力でなんとかできるかもしれない。
この辺りのバランスを取ると、もしかしたらpoetry-exportを実行するのがいいのかもとちょっと思いました。

(せっかく書いていただいたのに申し訳ないです 🙇 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>pythonではなくpython3
このパターンが確かにありましたね……
pythonコマンドから 実行バージョンを取得して、そのバージョンに応じたコマンドで poetry_export_all.py を呼び出すとかでしょうか
それかpoetry-exportは pre-commitのentryで呼び出してgit diffによる確認だけスクリプトを呼び出す形にするか
やりようはある気がするので探ってみます

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commitのentryで呼び出してgit diffによる確認だけスクリプトを呼び出す形

これで問題なさそうだったので poetry_export_check.py にリネームして、git diffだけPythonスクリプトで呼び出すようにしました
pythonコマンドがなくてpython3だけ入ってるということは考慮から外したのですが、問題ないでしょうか?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほどです!! 提案ありがとうございます!!

まあでも、「poetry-exportによるエラーの理由が分からずに混乱する人の数」よりも、「python3コマンドを使っていてエラーが発生し理由が分からずに混乱する人の数」の方が多いかもですかね。。。
だとしたらpoetry-export-checkの方はない方がいいかもです 🙇‍♂️

どの環境でも動く方法があればという感じかなと・・・!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

差分のチェックは bash コマンドから poetry_export_check.shを実行する方針にしました

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bashもwindowsだと動かないんですよね・・・・・・・。

ChatGPT君に聞いてみた感じ、なんか良い感じの方法はなさそうでした。
https://chat.openai.com/share/94cee641-dfe0-483d-b9f3-84085899db20
なにかの言語で書いたものをバイナリにビルドして~とかが紹介されていますが、流石に・・・。

せっかくチャレンジしてくださったのに申し訳ないです 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows10からbashが正式サポートされた認識でした……
消しておきます!

@FujisakiEx FujisakiEx force-pushed the issue_566 branch 9 times, most recently from 765c886 to db21bde Compare July 13, 2023 17:20
@FujisakiEx FujisakiEx changed the title Issue 566 Github Actions とpre-commitの適用 Jul 13, 2023
@FujisakiEx
Copy link
Contributor Author

@Hiroshiba
フィードバックに対応しました
hiroshibaさんのほうでpre-commit.ciの導入だけやってもらわないとダメかもです

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.

追加ありがとうございます!!

Comment on lines 43 to 52
- name: Add Path for ubuntu
if: matrix.os == 'ubuntu-20.04'
run:
echo "$HOME/.local/bin" >> $GITHUB_PATH

- name: Add Path for Windows
if: matrix.os == 'windows-latest'
run:
echo "C:\Users\runneradmin\AppData\Roaming\Python\Scripts" >> $GITHUB_PATH

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これもpip installでpoetry入れるようになったらいらないはず・・・?

.github/workflows/test.yml Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

pre-commit.ci

これは何でしょう、pre commitの内容をテストしてくれる感じでしょうか?
まあ同じことを Github Actions で行っているので、マストではないかなと思いました!
(でもあった方が便利だとは思います!)

@FujisakiEx
Copy link
Contributor Author

pre-commit.ci は Github上で確認してくれるようになるやつですね
All check have passedのところに今回追加したテストが並ぶ感じになります
そこら辺の判断はお任せします

@FujisakiEx
Copy link
Contributor Author

@Hiroshiba 再レビューお願いします!

@FujisakiEx
Copy link
Contributor Author

@Hiroshiba 再レビューお願いします!

Comment on lines 3 to 5
env:
POETRY_VERSION: "1.3.1"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

未使用な気がします!
バージョンはrequirements.txtにて固定しているので、ここに書いちゃうとバージョン不一致になってしまうかもです。

Comment on lines 3 to 5
repos:
- repo: https://github.com/python-poetry/poetry
rev: '1.3.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここもバージョン不一致になっちゃう可能性がありますね!
まあさほど問題ないのですが、もしrequirements.txtをpip installしたときに入るpoetryを使えるとちょっとかっこいいかもとか思いました。
難しそうなら一旦このままでも!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip install -r requirements.txtしてからpoetry exportを叩くように変更しました

@Hiroshiba
Copy link
Member

ほぼLGTMです!!!
いろいろ修正ありがとうございます!!!

@FujisakiEx
Copy link
Contributor Author

@Hiroshiba 再レビューお願いします

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!!!
ちょっと細かい部分をこちらで調整させて頂きます!!

PRありがとうございました!
こちらのコメントに対してしっかり対応頂けて嬉しかったです。

もしよかったらまだまだいろんなissueがありますので、なにかできそうなのがあればぜひ・・・!
https://github.com/VOICEVOX/voicevox_engine/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc

entry: poetry export --without-hashes -o requirements.txt
language: python
pass_filenames: false
stages: [commit]
Copy link
Member

@Hiroshiba Hiroshiba Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pysen-lintに合わせてstages: [push]にしようと思います! 🙇

Comment on lines 13 to 18
- id: pip-install
name: pip-install
entry: pip install -r requirements.txt
language: python
pass_filenames: false
stages: [commit]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip installが勝手に実行されるのは意図せず開発環境が壊れかねないので、一旦やめておく感じで・・・!

@Hiroshiba Hiroshiba merged commit c964f5e into VOICEVOX:master Jul 18, 2023
3 checks passed
@Hiroshiba Hiroshiba changed the title Github Actions とpre-commitの適用 Github Actions とpre-commitにpoetry exportを適用 Jul 18, 2023
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.

None yet

2 participants