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.

Sign up for GitHub

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

Jump to bottom

Add: ライブラリのダウンロードAPIを実装 #616

Merged
merged 26 commits into from
Feb 25, 2023
Merged

Add: ライブラリのダウンロードAPIを実装 #616

merged 26 commits into from
Feb 25, 2023

Conversation

sevenc-nanashi
Copy link
Member

内容

ダウンロードAPIを実装します。

関連 Issue

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

（なし）

その他

（なし）

@github-actions
Copy link

github-actions bot commented Feb 6, 2023 •
edited
Loading

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/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 150 9 coverage-94%
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/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 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 140 10 coverage-93%
voicevox_engine/utility/init.py 4 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/mutex_utility.py 10 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
TOTAL 1361 275 coverage-80%

@sevenc-nanashi
Copy link
Member Author

__gi_update_api.zip

テスト用API。

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review February 7, 2023 11:47
@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner February 7, 2023 11:47
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team February 7, 2023 11:47
@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Feb 7, 2023 •
edited
Loading

インストールを実装してみました。
root_dir/librariesにダウンロードされる仕様になっています。root_dir/libraries/[uuid]/library_info.jsonに情報が入ります。

テストAPI：__gi_update_api.zip

takana-v
takana-v reviewed Feb 16, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
run.py Outdated
Comment on lines 814 to 815
if not manifest.supported_features.manage_library:
raise HTTPException(status_code=501, detail="この機能は実装されていません")
Copy link
Member

Choose a reason for hiding this comment

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

エディタでエンジンの再起動を促されることになるので、エンジンでは500番台のエラーをあまり投げないようにしているはずです。
過去の例に合わせると、422や404辺りになるかなと思います。

Copy link
Member Author

Choose a reason for hiding this comment

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

404にしました。

Copy link
Member

@y-chan y-chan Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

エンジンでは500番台のエラーをあまり投げないようにしているはず

これちょっとドキュメント化しておきたいですね:eyes:
このコメントはあえて未解決にして、あとから参照できるようにしておきます

Copy link
Member

Choose a reason for hiding this comment

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

あ、これどこに書きましょう。

Copy link
Member

Choose a reason for hiding this comment

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

docs/APIの設計.mdみたいなのを用意すると良さそうと思いました！
このPRの範囲外にはなりそうなので、別でちょっとPRしてみます！

voicevox_engine/downloadable_library.py Outdated Show resolved Hide resolved
voicevox_engine/downloadable_library.py Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

@y-chan こちらのダウンロードAPIのPR、おそらく @y-chan さんがレビューするのが適任な気がしているのですが、おまかせしても良いでしょうか 👀

@y-chan
Copy link
Member

y-chan commented Feb 17, 2023

テスト期間でバタバタしていてあまりちゃんと見れる時間が取れていませんでした...

今日から春休みで時間も十分にあるので私の方で引き受けます...!

y-chan
y-chan reviewed Feb 19, 2023
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

おまたせしました...!
ほぼLGTMですが、いくつかメンテナンス上のコメント(と私の個人的な意見)を残しました...!
まあ、VOICEVOXとして本質的にやりたいことは、他のアプリケーションでもライブラリダウンロードを行えるようにするために、APIを決めたいだけで、API実装の詳細を詰めたいわけではないので、これで良いと思います...!

voicevox_engine/downloadable_library.py Outdated
Comment on lines 42 to 43
else:
raise HTTPException(status_code=404, detail="指定されたライブラリが見つかりません。")
Copy link
Member

Choose a reason for hiding this comment

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

公式にダウンロード可能なモデル以外を差し込みたい場合がある(COEIROINKでいうところのMYCOEIROINKとか)ので、一概に蹴るのは良くないかも、と思いつつ、VOICEVOXではそもそも使わないのでまあ一旦無視して良さそう。

voicevox_engine/engine_manifest/EngineManifest.py Show resolved Hide resolved
engine_manifest.json Show resolved Hide resolved
@sevenc-nanashi
Copy link
Member Author

多分APIが変わることは無いと思うので、エディタ側の実装もやってきます

y-chan
y-chan approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

LGTMです！

Hiroshiba
Hiroshiba reviewed Feb 19, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 19, 2023
View reviewed changes
run.py Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 19, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 19, 2023
View reviewed changes
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.

ひと通り見てみました！！
できることが広がりそうで、良いですね！！！

@y-chan y-chan mentioned this pull request Feb 19, 2023
Open
21 tasks
@Hiroshiba
Copy link
Member

こちらどうしましょう･･･？

やはりプロキシをかませないと行けない仕様となって気軽に動かせないコードになるのは大変かなと思いました。
改修するにしてもまず再現になっちゃうので、このPRの段階で、ファイル読み込みでもできるように戻すのが丸いのかなと思っています。
engine_manifestのdownloadable~url,pathを復活させ、URLがあればそちらを読み、pathがあればそちらを読む形かなと思いました。
手間かかってしまいますが、何卒。。

@sevenc-nanashi
Copy link
Member Author

あ、pushできてない…
あの後、ファイルを読み込むコードにして、今のHTTPリクエストするコードはコメントにしてます（フォークする人のため）
pushしてきます

Hiroshiba
Hiroshiba reviewed Feb 23, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あ、テストが落ちてそうでした！

@sevenc-nanashi
Copy link
Member Author

レビューいただいたところは修正しましたー。

Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
run.py
@@ -180,6 +180,7 @@ async def block_origin_middleware(request: Request, call_next):
engine_manifest_loader = EngineManifestLoader(
root_dir / "engine_manifest.json", root_dir
)
library_manager = LibraryManager(get_save_dir() / "installed_libraries")
Copy link
Member

Choose a reason for hiding this comment

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

ここ将来的にはVVPPのvvpp-enginesと合わせたいですね！
vvlibとなるなら、そういう感じに。

Hiroshiba
Hiroshiba approved these changes Feb 25, 2023
View reviewed changes
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！！！

@Hiroshiba Hiroshiba merged commit 8bc4d47 into VOICEVOX:master Feb 25, 2023
@takana-v takana-v mentioned this pull request Mar 3, 2023
Merged
@Hiroshiba
Copy link
Member

製品版エンジンで動かなくなっていたのでpreviewリリースを作りました！

@Hiroshiba Hiroshiba mentioned this pull request Apr 18, 2023
Closed
@sabonerune sabonerune mentioned this pull request Dec 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-chan y-chan y-chan approved these changes

@Hiroshiba Hiroshiba Hiroshiba approved these changes

@takana-v takana-v Awaiting requested review from takana-v

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sevenc-nanashi @Hiroshiba @y-chan @takana-v