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

追加: GET / API にポータルページを配置 #1169

Merged
merged 8 commits into from
May 5, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Apr 2, 2024

内容

概要: GET / API にポータルページを配置した

API が提供する GET /settingGET /docs およびライセンスへのリンクを含むポータルページを実装した。

関連 Issue

resolve #778

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

image

@tarepan tarepan requested a review from a team as a code owner April 2, 2024 12:43
@tarepan tarepan requested review from Hiroshiba and removed request for a team April 2, 2024 12:43
Copy link

github-actions bot commented Apr 2, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 283 122 coverage-57%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/app/init.py 0 0 coverage-100%
voicevox_engine/app/dependencies.py 9 0 coverage-100%
voicevox_engine/app/routers/init.py 0 0 coverage-100%
voicevox_engine/app/routers/preset.py 37 4 coverage-89%
voicevox_engine/app/routers/setting.py 23 3 coverage-87%
voicevox_engine/app/routers/speaker.py 59 5 coverage-92%
voicevox_engine/app/routers/tts_pipeline.py 121 29 coverage-76%
voicevox_engine/app/routers/user_dict.py 60 29 coverage-52%
voicevox_engine/cancellable_engine.py 97 75 coverage-23%
voicevox_engine/core/init.py 0 0 coverage-100%
voicevox_engine/core/core_adapter.py 81 6 coverage-93%
voicevox_engine/core/core_initializer.py 60 30 coverage-50%
voicevox_engine/core/core_wrapper.py 228 160 coverage-30%
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 2 coverage-97%
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 11 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 72 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 82 2 coverage-98%
voicevox_engine/preset/init.py 0 0 coverage-100%
voicevox_engine/setting/Setting.py 9 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 20 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 268 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_utility.py 6 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 2527 521 coverage-79%

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.

良いですね!!!

run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
run.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Apr 15, 2024

マルチエンジン絡みの質問です。

API は VV API としてマルチエンジンに配慮した設計が必要、と認識しています。同時に、内部実装はエンジンごとに違うので特段の配慮不要、とも認識しています。
/ API は「HTMLを返す」が API 仕様で、HTMLの記述内容は内部実装依存です。
上記レビューでの埋め込みリンクも後者に属すると思うのですが、どの辺が懸念点でしょうか?

@Hiroshiba
Copy link
Member

たしかに懸念点が不明瞭でした!

コードを読まないとマルチエンジンのためにどこを変更したら良いかわからない点かなと!
あとマルチエンジンのドキュメントに「どこを変えれば良いか」とか案内することとかを考えると、変更しないといけないとこは集約されてた方が説明しやすい、とか…!

@tarepan
Copy link
Contributor Author

tarepan commented May 1, 2024

エンジン固有設定は設定ファイルへ集約できるなら集約する(例: engine_manifest.json)、が基本方針ということですね。理解しました。

ライセンス情報はこちら(#778 (comment) )の suggestion に従って追加していますが、ライセンス情報はポータルに無しにするのも手と考えています。

ライセンス情報をいれる場合、エンジンごとに要件が異なるため使い回せるコードとするのが難しいと考えます。
例えば企業製有料 ENGINE を作る場合、製品版ソフトウェアのライセンス(=利用規約)はホームページ上に掲載した利用規約へのリンクにしたいです(サービス規約更新の柔軟性が上がる)。となると現行の engine_manifest.json には相当する attribute が無さそうな気がします。
集約できないのであれば、エンジン間で使い回せない前提で割り切って VOICEVOX ENGINE 向けにリンクをハードコードしても良いと考えます。

どちらの方針がよいでしょうか?

@Hiroshiba
Copy link
Member

Hiroshiba commented May 3, 2024

@tarepan なるほどです!

無しで良いと思います!

一方で別にあっても良いと思います。
ここに利用規約が書いてるのは別に必ず踏んでほしいという意図ではなく、ちょっと確認したいときに便利くらいの感じになると思います。

載せる場合、利用規約はまあmanifestに置いてあるterms_of_serviceのmarkdownを表示する形が丸い気がしました。
リンクが良い場合はこのmarkdownにリンクを書いてもらう感じになる気がします。
よりリッチな案内もできるかもですが、まあそういうのはissueやPRが立ってからで良いかなと。

載せても載せなくてもどちらでも良いと思います!検討ありがとうございます!!

@tarepan tarepan requested a review from a team as a code owner May 3, 2024 07:40
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 3, 2024 07:40
@tarepan
Copy link
Contributor Author

tarepan commented May 3, 2024

無しで良いと思います

👍️
削除します。

リンクが良い場合はこのmarkdownにリンクを書いてもらう感じ

👍️
なるほどです、合理的な方法だと思います。
将来実装することがあればこちらが良さそうです、提案ありがとうございます!


@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!!

ちょっと一部だけ調整させていただきます!

run.py Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 7ca6756 into VOICEVOX:master May 5, 2024
3 checks passed
@tarepan tarepan deleted the add/portal branch May 6, 2024 03:36
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.

GET / にポータルページ
2 participants