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

修正: fastapi-slim の暗黙的な依存があることを pyproject.toml にコメントとして書く #1400

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 16, 2024

内容

概要: fastapi-slim の暗示的依存を pyproject.toml へ明記して修正

fastapi-slim は複数の optional dependency をもち、一部機能を利用する場合に限ってこれらライブラリを必要とする。これら optional dependency は fastapi-slim の管理外にあるため、fastapi-slim を導入しても poetry.lock でこれらライブラリのバージョンは管理できない。
VOICEVOX ENGINE はその一部機能を利用するため、pyproject.toml で明示的に optional dependency を管理している。それと同時にこれらライブラリのいくつかは(fastapi-slim 経由でなく)ENGINE そのものでも利用されている。

この管理手法には危険性がある。それは意図せず暗示的依存を pyproject.toml から消してしまうことである。
ENGINE でも直接依存しているライブラリの場合、ENGINE が直接依存しなくなったタイミングで(まだ fastapi-slim が暗示的依存しているのに)そのライブラリを pyproject.toml から消してしまう可能性がある。

これを防ぐため pyproject.toml には NOTE が付けられている。しかし NOTE の意図が伝わりづらい文面となっていたため、この NOTE を誤って消すケースが発生している (#1381)。また一部 NOTE は既に消されている。

このような背景から、fastapi-slim の暗示的依存を pyproject.toml へ明記する修正を提案します。

関連 Issue

ref #1381 (review)

@tarepan tarepan requested a review from a team as a code owner June 16, 2024 01:06
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 16, 2024 01:06
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.

良さそう…だと思ってたのですが、ちょっとコメントがいくつか!

starletteもかも・・・・・・・・?
https://fastapi.tiangolo.com/#dependencies
…なかなかややこしいですね!!

slimじゃないstandard(何もpostfixがない)fastapi使えばこのあたり入ってくるっぽみ?
となるとfastapiをinstallする形にするのが手っ取り早いかもとちょっと思いました!
余計なのが入ってくるので要確認かもですが。

昔はallか今のslimしかなかったのでslim側使ってたけど、バージョン111でstandardができたって感じですかね。(ちゃんと確認してないですが)

暗示的依存

全然関係ないのですが、プログラミングでimplicitの訳を"暗示"とするのは見たことがないかもです。
Pythonドキュメントも"暗黙"でした。
https://docs.python.org/ja/3/library/stdtypes.html#:~:text=%E5%88%97%E3%83%AA%E3%83%86%E3%83%A9%E3%83%AB%E3%81%AB-,%E6%9A%97%E9%BB%99%E3%81%AB%E5%A4%89%E6%8F%9B,-%E3%81%95%E3%82%8C%E3%81%BE%E3%81%99

暗示は「ほのめかす」感じなので、暗黙のが直感的に正しい気もします。
google検索で「暗示 依存」と「暗黙 依存」を検索すると、用語のわかりやすいかも。

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Jun 17, 2024

starletteもかも

starlette は required で常に入っているような?

https://github.com/tiangolo/fastapi/blob/d92a76f3152185ba19d3ee89f8c11be13b3205ce/pyproject.toml#L54-L77

fastapiをinstallする形にするのが手っ取り早い

#1077 で一旦 NoGo になったとの認識です。当時と状況が変わっているので、以下で再検討しました。

standard を入れた場合、不使用だけど入るライブラリは ujson / orjson / email_validator / uvicorn[standard] になりそうです。#1077 での検討に基づくと、影響がありそうなのは uvicorn[standard] あたり?
VOICEVOX ENGINE の依存周りがかなり片付いたので、#1077 で挙がっていた移行の阻害因子は全部解決していそうです。

@Hiroshiba
standard へ移行すれば本 PR の問題は一掃できるので、私は移行に賛成です。
実際に PR 作成して評価する方針で良いでしょうか?


プログラミングでimplicitの訳を"暗示"とするのは見たことがない

👍️

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 17, 2024

ひとまずコメントだけ!

starlette は required で常に入っているような?

これってfastapi-slimにくっついてくるやつなんですかね・・・?
jinja2とかも入ってるように見えますが・・・

#1077 で一旦 NoGo になったとの認識です。

以前話してたのはallで今回はstandardだから違う・・・と思ってました!
依存ライブラリの項目はallとstandardで全く一緒・・・?

だとしたら一度 #1077 で検討した項目ですね!! すみません 🙇

fastapiの意図がわかりませんが、fastapiが何を標準とするか変えようとしてる過渡期な気がしなくもないですねぇ。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 17, 2024

現状

#1403 での検討により、#1077 ライクな fastapi-slim[standard] 移行は NoGo となった。

レビュー応答


starlette は required で常に入っているような?

これってfastapi-slimにくっついてくるやつなんですかね・・・?
jinja2とかも入ってるように見えますが・・・

上記のFastAPI 依存ライブラリリスト(再掲)は optional dependency のリストであり、これは jinja2 を含み starlette を含んでいません。実際にインストールした fastapi-slim の pip list をチェックをしたところ、 starlette が含まれ jinja2 は含まれていませんでした。
なので依存リスト的にも実挙動的にも starlettefastapi-slim に付いてくるものです。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re2-review よろしくお願いします。

@Hiroshiba
Copy link
Member

なるほどです!! なにか勘違いしていたようでした、すみません 🙇
たしかにstarletteはrequiredでした。

@Hiroshiba Hiroshiba changed the title 修正: fastapi-slim の暗示的依存を pyproject.toml へ明記 修正: fastapi-slim の暗黙的な依存を pyproject.toml に書く Jun 17, 2024
@Hiroshiba Hiroshiba changed the title 修正: fastapi-slim の暗黙的な依存を pyproject.toml に書く 修正: fastapi-slim の暗黙的な依存があることを pyproject.toml にコメントとして書く Jun 17, 2024
@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re3-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!

早くstandardに依存移したいですね。
fastapi側からドキュメント化やスタンスさえ提供してくれれば。

@Hiroshiba Hiroshiba merged commit 1f4e00a into VOICEVOX:master Jun 18, 2024
4 checks passed
@tarepan tarepan deleted the fix/fastapi_implicit_dep branch June 19, 2024 02:17
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.

2 participants