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

requirements を Poetry で管理する #535

Merged
merged 17 commits into from
Jan 9, 2023

Conversation

sarisia
Copy link
Contributor

@sarisia sarisia commented Dec 25, 2022

内容

requirements を Poetry で管理し, 以下の課題を解決します:

  • pip-compile を実行する OS によって requirements.txt の出力結果が変わっていましたが, PR ではどの環境で実行しても同じ出力が得られるようなります
    • 特定の OS に依存するパッケージに対し, 自動で --platform 指定が入るため, 全ての OS で同一の requirements.txt を利用できます
  • pip >= 22 の際に pip-tools がエラーで実行できませんでしたが, poetry は pip のバージョンによらず実行可能です

関連 Issue

fixes #220

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

その他

  • 不要となった依存 pip-tools を削除しました
  • 成果ファイル名は変わらず requirements.txt, requirements-dev.txt, requirements-test.txt を維持していますので, CI/CD は壊れていないと思います
  • requirements.in などで直接指定されていた依存関係のバージョンは (patch 含め) 変更されていませんが. indirect な依存関係のバージョンが部分的にアップグレードされているようです. 手元で run.py 実行 + 音声合成程度の簡単な動作テストは実施しましたが, 追加で何かあればご教示いただければと思います!

tasks

@takana-v
Copy link
Member

takana-v commented Dec 25, 2022

ref #149


以前poetryの導入を検討した際はバグによりwindowsで正常に動作しなかったため導入を見送りましたが、現在そのバグは修正されているようです。

@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

git からの pip install だとハッシュ検証失敗するんですね… 出力ファイルからハッシュを削除する形で修正しようと思います。

@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

black (pysen の依存) のバグで落ちているようですね. (psf/black#2964)

以下の複数の選択肢があるのですが, いかがでしょうか?

  1. click のみ明示的に現在のバージョンに固定する (test グループへの依存関係の追加)

  2. black >= 22.3.0 で修正されているようなので, pysen 側で依存関係が修正されているようなので, pysen をアップグレードする (0.9.1 -> 0.10.2) (Force lint extras to use older click pfnet/pysen#13)

  3. pysen のバージョンを維持するが, lint extras の利用をやめて明示的に black, flake8, isort, mypy のバージョンを指定する方法もあります: https://github.com/pfnet/pysen#install-pysen-with-your-choice-of-linter-versions

@Hiroshiba
Copy link
Member

PRありがとうございます!とても丁寧な進行でとても助かっています!

以下の複数の選択肢があるのですが, いかがでしょうか?

2のpysenバージョンアップが一番素直なのかなと思いました!

@Hiroshiba
Copy link
Member

Poetryとても便利ですね!感動しました。

特定の OS に依存するパッケージに対し, 自動で --platform 指定が入るため, 全ての OS で同一の requirements.txt を利用できます

まさにOS依存でとても困っていたことがあったので期待しています・・・!

もしご存知だったら知りたいのですが、例えばwin/macで別のライブラリに依存しているものをwinもしくはmacでインストールした場合、win/mac両方か、あるいは片方だけのライブラリがlockやrequirements.txtに書かれるのでしょうか 👀

@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

#535 (comment)
もしご存知だったら知りたいのですが、例えばwin/macで別のライブラリに依存しているものをwinもしくはmacでインストールした場合、win/mac両方か、あるいは片方だけのライブラリがlockやrequirements.txtに書かれるのでしょうか 👀

この場合, Win/Mac 両方のライブラリが poetry.lock に追加されます. requirements.txt 3ファイルは poetry.lock を忠実に反映しますので, こちらにも両OSのライブラリが記載されます.

それぞれのライブラリのインストールが各OSで行われるか, 行われないかの条件が requirements.txt 上に以下のように自動で指定されます:

https://github.com/VOICEVOX/voicevox_engine/pull/535/files#diff-2b4945591edfeaa4cf4d3f155e66d4b43d1bda7a55d881d5cf3107f1b05abbbcR15-R26

@Hiroshiba
Copy link
Member

別OSのも勝手に記載してくれるんですね!!
相当便利です、助かります!!

@Hiroshiba
Copy link
Member

あ、もしかしたらflake8のバージョンが上がってエラーになってますね・・・・・ 🙇‍♂️

@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

pysen 依存の flake8-bugbear が上がった結果、新規ルールが追加されたのでそれに違反して落ちてますね… 何度も落としてしまいすみません💦 (PyCQA/flake8-bugbear#273 こっちでした PyCQA/flake8-bugbear@592b722)

pysen では flake8-bugbear のバージョン指定がありませんので、バージョンを落として以前のルールに戻すこともできるのですが、バグ防止という意味では指摘内容からソースコードを修正した方が良い気がしています…!

@Hiroshiba
Copy link
Member

該当箇所を改修すのに賛成です!
直せそうだったらおまかせできると嬉しいです!難しそうでしたらそのままにして頂ければ・・・!

@sarisia
Copy link
Contributor Author

sarisia commented Dec 25, 2022

対応できますので別PRで対応します、よろしくお願いします!

@sarisia sarisia marked this pull request as draft December 26, 2022 15:50
@sarisia
Copy link
Contributor Author

sarisia commented Dec 29, 2022

PR を master に対して rebase しました.

今度はライセンス取得 (generate_licenses.py) で落ちているのですが, 原因は platformdirs パッケージのライセンスファイルが LICENSE.txt から LICENSE にリネームされたことのようです.

ファイルのリネームだけで壊れてしまう原因が正直わかっていませんが, 以下の対処法のどちらが良さそうでしょうか?

  1. pip-licenses を v4 系にアップグレードする (現在は v3) (正常に generate_licenses.py が走ることを確認しています)
  2. platformdirs を 元の 2.4.0 にロックする
  3. generate_licenses.py にライセンス取得処理を追加する

@sarisia sarisia marked this pull request as ready for review December 29, 2022 09:25
@sarisia
Copy link
Contributor Author

sarisia commented Dec 29, 2022

pip-licenses v4 はライセンス周りの問題があるようですね (#544). 対処法2でロックバージョンを戻そうと思います.

@github-actions
Copy link

github-actions bot commented Dec 29, 2022

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/model.py 155 7 coverage-95%
voicevox_engine/mora_list.py 4 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/PresetLoader.py 34 1 coverage-97%
voicevox_engine/preset/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 206 166 coverage-19%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 57 49 coverage-14%
voicevox_engine/synthesis_engine/synthesis_engine.py 133 12 coverage-91%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 129 6 coverage-95%
voicevox_engine/utility/init.py 3 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
TOTAL 1238 262 coverage-79%

@Hiroshiba
Copy link
Member

他issueもチェックして頂いたりと、いろいろとありがとうございます!!助かります!!

ちょっと実際にリニューアルされたREADME見つつ動かしてみたいところなのですが、今帰省していていつもの開発環境がなく・・・。
@takana-v さん・ @aoirint さん、もしよければお願いしても良いでしょうか 🙇‍♂️

@takana-v
Copy link
Member

#531 でjinja2が追加された結果、コンフリクトが発生しているみたいです。
お手数ですが、コンフリクト解消をお願いします:bow:

@sarisia
Copy link
Contributor Author

sarisia commented Dec 31, 2022

ありがとうございます, 対応しました!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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です!!READMEだけ!

めちゃくちゃいいなと思いました!
できるかわかってないのですが、lockとかrequirements.txtとかがずれてないかを確認するActionsとかあっても面白いかもですね!!

@sarisia
Copy link
Contributor Author

sarisia commented Jan 5, 2023

#535 (review)
できるかわかってないのですが、lockとかrequirements.txtとかがずれてないかを確認するActionsとかあっても面白いかもですね!!

diff 取ればできそうですね!テスト時に回せるようにしたら安心できそうです.

こちらのマージ後にフォローアップ Issue などで対応したいと思います, ありがとうございます!

@sarisia sarisia requested a review from a team as a code owner January 5, 2023 15:48
@sarisia sarisia requested review from y-chan and removed request for a team January 5, 2023 15:48
sarisia and others added 2 commits January 6, 2023 00:48
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@sarisia sarisia marked this pull request as draft January 5, 2023 15:52
@sarisia
Copy link
Contributor Author

sarisia commented Jan 5, 2023

以下の修正を行いました

(ビルド成果物に Poetry 関係のライブラリは入らないはずなので不要な気がしていますが, 一応...)
新たに追加された依存関係 (d386407) のライセンスは以下の通りで, README.md に記載されていた利用可能なライセンスの範囲かと思いますが, 念のためレビュー頂けますでしょうか.

package name version license
attrs 22.2.0 MIT License
CacheControl 0.12.11 Apache Software License
cleo 2.0.1 MIT License
crashtest 0.4.1 MIT License
cryptography 39.0.0 Apache Software License; BSD License
dulwich 0.20.50 Apache Software License
html5lib 1.1 MIT License
importlib-metadata 4.13.0 Apache Software License
importlib-resources 5.10.2 Apache Software License
jaraco.classes 3.2.3 MIT License
jeepney 0.8.0 MIT License
jsonschema 4.17.3 MIT License
keyring 23.13.1 MIT License
lockfile 0.12.2 MIT License
more-itertools 9.0.0 MIT License
msgpack 1.0.4 Apache Software License
packaging 22.0 Apache Software License; BSD License
pexpect 4.8.0 ISC License (ISCL)
pkginfo 1.9.4 MIT License
pkgutil-resolve-name 1.3.10 MIT License
platformdirs 2.6.2 MIT License
poetry 1.3.1 MIT License
poetry-core 1.4.0 MIT License
poetry-plugin-export 1.2.0 MIT License
ptyprocess 0.7.0 ISC License (ISCL)
pyrsistent 0.19.3 MIT License
rapidfuzz 2.13.7 MIT License
requests-toolbelt 0.10.1 Apache Software License
SecretStorage 3.3.3 BSD License
shellingham 1.5.0.post1 ISC License (ISCL)
tomli 2.0.1 MIT License
tomlkit 0.11.6 MIT License
trove-classifiers 2022.12.22 Apache Software License
webencodings 0.5.1 BSD License
zipp 3.11.0 MIT License

@sarisia sarisia marked this pull request as ready for review January 5, 2023 18:36
README.md Show resolved Hide resolved
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!!!

ありがとうございました!!
@takana-v さんor @y-chan さんもレビュー頂けると・・・!

Copy link
Member

@takana-v takana-v left a comment

Choose a reason for hiding this comment

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

LGTM!

@takana-v takana-v merged commit 26b91bd into VOICEVOX:master Jan 9, 2023
@sarisia sarisia deleted the manage-deps-with-poetry branch January 9, 2023 16:16
@Hiroshiba Hiroshiba removed the request for review from y-chan January 9, 2023 16:46
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.

requirements系の管理をCross Platform対応が必要か?
3 participants