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

追加: poetry.lock 形式チェック CI #1081

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Feb 26, 2024

内容

poetry.lock 形式が poetry バージョン と一致するかチェックする CI を追加

関連 Issue

resolve #750

@tarepan tarepan requested a review from a team as a code owner February 26, 2024 13:13
@tarepan tarepan requested review from y-chan and removed request for a team February 26, 2024 13:13
@tarepan
Copy link
Contributor Author

tarepan commented Feb 26, 2024

エラー

content-hash の不一致によりエラー発生。

-content-hash = "69e00cbb0d47334a30dcddf8b483e298cc3397fa89df2f627841b7cbf43bc191"
+content-hash = "46753e9c8b1341ed82fc1e28398e050c12d981308147c7ede86ff14684f32d84"

現在の poetry.lock が既に壊れている...?(.lock 生成以降に pyproject.toml に変更が入った?)

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 26, 2024

あ、すでに壊れてるんだと思います!

lockに対する変更Aのあとに変更Bがあったとき、変更Bはmerge masterしたあともう一度lockを更新する運用をすしないとな感じです。
完全に忘れてました。(ちょっと面倒なんですよね…)

lockの更新はどうするんだったかな。。
lock --no-update…?

@sarisia
Copy link
Contributor

sarisia commented Feb 26, 2024

poetry 関連を少し見ていたのですが, 実はこれをやる pre-commit hook が公式から出てますので, それを pre-commit 設定に入れてあげるといいかと思います

(同様に, #716 の内容についても公式に pre-commit が用意されています)

pre-commit hooks | main | Documentation | Poetry - Python dependency management and packaging made easy

@aoirint
Copy link
Member

aoirint commented Feb 27, 2024

lockに対する変更Aのあとに変更Bがあったとき、変更Bはmerge masterしたあともう一度lockを更新する運用

で同じ問題がありましたね。このPRが取り込まれれば、CIで気づけるようになるかなと思います(手間ではありますが、pyproject.tomlpoetry.lockの間のバージョン制約の整合性は保たれる)。

poetry.lockの形式チェックについては、Poetry 1.5で削除されたpoetry.lock内のcategoryは、このPRのCIで対応できると思います。

ちなみに、poetry.lockファイルの1行目にPoetryのバージョン情報がTOMLコメントとして出力されますが、手元(Poetry 1.3.2、Poetry 1.8.0、Poetry 1.8.1)で試したところpoetry lock --no-updateを実行しても、poetry.lockファイルのその他の箇所が変更されない場合、このコメント行は更新されないようでした。更新が確認できたのは、content-hashに不一致があるか、categoryの削除がある場合です。

-# This file is automatically @generated by Poetry 1.6.1 and should not be changed by hand.
+# This file is automatically @generated by Poetry 1.8.1 and should not be changed by hand.

ハックとして、poetry.lockファイルから手動でcontent-hashの行を削除すると、poetry lock --no-updatecontent-hashの再生成と1行目コメントの更新が行われます( python-poetry/poetry#496 (comment) のユーザワークアラウンドの改変 )。このコメント行 python-poetry/poetry#7339 が取り込まれたPoetry 1.4以上なら、Poetryバージョンを厳密に制限できそうですが、公式でないトリッキーな方法で不安があるので、導入するかは悩ましいかなと思います。

  • 実装する場合: content-hashを意図的に壊してpoetry lock --no-updateして、壊す前と差分がないことを確認する

@aoirint
Copy link
Member

aoirint commented Feb 27, 2024

pre-commit hook

poetry check --lockについて、使ったことがないんですが、Poetryがpoetry.lockの後方互換性を維持する限りは VOICEVOX/voicevox_core#655 のようにチェックが通りそうなので、バージョン固定には使えないかもと思っています。整合性チェックの効果はあると思います。

Copy link

github-actions bot commented Feb 28, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 513 218 coverage-58%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core/init.py 0 0 coverage-100%
voicevox_engine/core/core_adapter.py 81 12 coverage-85%
voicevox_engine/core/core_initializer.py 59 30 coverage-49%
voicevox_engine/core/core_wrapper.py 257 183 coverage-29%
voicevox_engine/dev/init.py 0 0 coverage-100%
voicevox_engine/dev/core/init.py 0 0 coverage-100%
voicevox_engine/dev/core/mock.py 65 4 coverage-94%
voicevox_engine/dev/tts_engine/init.py 0 0 coverage-100%
voicevox_engine/dev/tts_engine/mock.py 28 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifest.py 36 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 0 0 coverage-100%
voicevox_engine/library_manager.py 92 4 coverage-96%
voicevox_engine/metas/Metas.py 36 0 coverage-100%
voicevox_engine/metas/MetasStore.py 28 1 coverage-96%
voicevox_engine/metas/init.py 0 0 coverage-100%
voicevox_engine/model.py 180 3 coverage-98%
voicevox_engine/morphing.py 71 4 coverage-94%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 2 coverage-98%
voicevox_engine/preset/init.py 0 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 17 0 coverage-100%
voicevox_engine/setting/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/kana_converter.py 88 1 coverage-99%
voicevox_engine/tts_pipeline/mora_mapping.py 7 0 coverage-100%
voicevox_engine/tts_pipeline/phoneme.py 34 0 coverage-100%
voicevox_engine/tts_pipeline/text_analyzer.py 146 6 coverage-96%
voicevox_engine/tts_pipeline/tts_engine.py 267 9 coverage-97%
voicevox_engine/user_dict/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/user_dict/user_dict.py 146 12 coverage-92%
voicevox_engine/utility/init.py 0 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2463 575 coverage-77%

@tarepan
Copy link
Contributor Author

tarepan commented Feb 28, 2024

これをやる pre-commit hook が公式から出てます

@sarisia さん情報提供ありがとうございます!hook化はリファクタリングという意味で有用そうです。
hookで出来ることはコマンドで実装できるため、本 PR ではコマンドでひとまず書こうと考えています。
現実装の hook 化含め、hook に関する個別 issue を建てました (#1090) ので、そちらで更に議論を深められればと思います。

content-hash 未更新の pre-commit による予防はまた別の議題となるため、こちらも個別 issue (#1091) を建てました。そちらで更に議論を深められればと思います。


みなさんの検証により、本 PR の実装だと以下の状況にあると認識しています:

  • できる
    • content-hash チェック(== pyproject.toml 内 dependencies ハッシュ値が lock へ反映されているか確認)
    • lockファイル形式チェック(例: category
  • できない
    • lockファイル冒頭 automatically @generated by Poetry 1.8.1 のバージョン番号検証
      • @aoirint さん提案の「content-hash 破壊再生成」を導入で対応可能

#750 は「できない」含めた解消を目的としているため、本 PR は #750 の partially resolve となるかと思います。
一旦 partially resolve を実現し、後続 PR で content-hash 破壊法の導入検討ができればと思います。

@Hiroshiba
この方針でどうでしょうか? Go であれば re-review よろしくお願いします。

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 28, 2024

@aoirint @tarepan ありがとうございます!

ちょっと認識ずれてるかもなのですが、1行目のチェックは厳密にこだわらなくてもメンテナンスに不都合はないかもと思いました!

一旦目的を整理すると、lockファイルのチェックでやりたいことは「ライブラリのバージョンがpoetryのバージョン違いにより異なることの防止」だと思います。
つまりまあ、ライブラリのバージョンがそのpoetryバージョンで変わってないなら、別に問題はないかもです。

他の観点として、pre-commitではエラーにならないのにCIではエラーになっちゃう、とかであればちょっと問題かもです。

(なんか認識おかしかったりしてたらご指摘いただけると。。 🙇 )

@tarepan
Copy link
Contributor Author

tarepan commented Feb 29, 2024

@Hiroshiba

(#750 より引用)
Poetryの更新でpoetry.lockのフォーマットが変わった場合に、項目の削除やバージョン文字列の更新に伴い、poetry.lockに不要な差分が生じる問題が起きます。
不要な差分が生じないように

#750 の直接の意図・目的は「ライブラリの安定性」ではなく「開発環境安定化によるレビューコスト減 + 潜在バグ源潰し」と認識しています。
潜在バグ源潰しの結果の「1つ」として、意図しないライブラリバージョン変更の防止があるという理解です。
lockファイル1行目の不変性チェックは poetry バージョンの一致、つまり開発環境安定化の証明であるため、この目的のためには有用だと考えます。

#750 の目的がそもそも過剰」という議論はありうると思います。
その場合、この PR でなく #750 で更なる整理をおこなうのが最適そうです(VOICEVOXは「PR は issue での同意を前提にする」という建付けなので、前提変更は issue 側が相応しい)

@Hiroshiba
Copy link
Member

なるほどです!
個人的には、poetryが変更不要と判断した1行目のバージョンを敢えて書き換えるワークアラウンドを入れるほどではないかな、と思ってます。
おそらく問題が発生することは無いので(ここの認識が違うかも?)

@aoirint さんの意見もお聞きできると🙇

@aoirint
Copy link
Member

aoirint commented Mar 1, 2024

@Hiroshiba

lockファイルのチェックでやりたいことは「ライブラリのバージョンがpoetryのバージョン違いにより異なることの防止」

#750 の直接の意図・目的は「ライブラリの安定性」ではなく「開発環境安定化によるレビューコスト減 + 潜在バグ源潰し」

poetryが変更不要と判断した1行目のバージョンを敢えて書き換えるワークアラウンドを入れるほどではない

認識は合っていると思います。
このPRはその機能を入れずにマージということでいいと思います。
#750 も、このPRでの議論を踏まえると、別案なければこのPRの現状以上のことはしないという形で別途closeという方向性になるかなと思います。

@tarepan
Copy link
Contributor Author

tarepan commented Mar 1, 2024

ワークアラウンドを入れるほどではないかな

PRの現状以上のことはしないという形で別途close

👍
必要範囲は本PRでカバー可能ということで、本PRを resolve #750 へ変更しました。

@Hiroshiba
この方向性で問題なければ 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!!

@aoirint さんもありがとうございました!!

.github/workflows/test.yml Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit c97806b into VOICEVOX:master Mar 1, 2024
4 checks passed
@tarepan tarepan deleted the add/safe_poetry_lock branch March 1, 2024 10:16
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.

異なるバージョンのPoetryによる操作を防ぎたい
4 participants