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

vvlib_manifest.jsonをバリデートするようにする #694

Merged
merged 17 commits into from
Jun 20, 2023
Merged

vvlib_manifest.jsonをバリデートするようにする #694

merged 17 commits into from
Jun 20, 2023

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Jun 16, 2023

内容

題の通り

関連 Issue

@y-chan y-chan requested a review from a team as a code owner June 16, 2023 12:02
@y-chan y-chan requested review from Hiroshiba and removed request for a team June 16, 2023 12:02
@y-chan y-chan force-pushed the feat/validate-library-manifest branch from 959770b to 039a2b0 Compare June 16, 2023 12:16
@github-actions
Copy link

github-actions bot commented Jun 16, 2023 •
edited
Loading

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 2 1 coverage-50%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 3 1 coverage-67%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/downloadable_library.py 91 4 coverage-96%
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 160 9 coverage-94%
voicevox_engine/mora_list.py 5 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 3 1 coverage-67%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 19 0 coverage-100%
voicevox_engine/setting/init.py 3 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 144 11 coverage-92%
voicevox_engine/utility/init.py 5 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 9 2 coverage-78%
voicevox_engine/utility/mutex_utility.py 11 1 coverage-91%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
TOTAL 1514 288 coverage-81%

Hiroshiba
Hiroshiba reviewed Jun 16, 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.

validate、便利ですね！
ちょっと気になったことがあったのでいくつかコメントしました！

voicevox_engine/downloadable_library.py Show resolved Hide resolved
voicevox_engine/downloadable_library.py Outdated Show resolved Hide resolved
voicevox_engine/downloadable_library.py Outdated Show resolved Hide resolved
voicevox_engine/engine_manifest/EngineManifest.py Show resolved Hide resolved
voicevox_engine/engine_manifest/EngineManifest.py
@@ -55,4 +55,5 @@ class EngineManifest(BaseModel):
terms_of_service: str = Field(title="エンジンの利用規約")
update_infos: List[UpdateInfo] = Field(title="エンジンのアップデート情報")
dependency_licenses: List[LicenseInfo] = Field(title="依存関係のライセンス情報")
supported_vvlib_manifest_version: str = Field(title="エンジンが対応するvvlibのバージョン")
Copy link
Member

Choose a reason for hiding this comment

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

将来エディターとエンジンのバージョンが分かれていって、エンジンのほうがバージョンが低いことがあり得るようになってくる気がしています。（そんなことはないかも･･･？）
このエンジンマニフェストは互換性を保てるように実装していく必要がでてきそうな気がします。

ここはデフォルト値をNoneにするのはどうでしょう？
あるいはこのmanifest_versionを上げるかかなと。

Copy link
Member Author

Choose a reason for hiding this comment

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

エンジンのほうがバージョンが低いことがあり得る

私もあり得ると思っています。

このエンジンマニフェストは互換性を保てるように実装していく

これについては、以前の破壊的変更(supported_featuresのmanage_library)にも同様のことが言えると思っていて、同時に修正したいので一旦このPRではOptionalを付けない方針で行きたいと思っています。
どうでしょうか？

Copy link
Member

@Hiroshiba Hiroshiba Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

一旦このPRではOptionalを付けない方針

良いと思います！！
（これ系いくつかあるんですが、時間空くので結構忘れちゃいそうなんですよね。。 😇 ）

run.py Outdated Show resolved Hide resolved
@y-chan y-chan force-pushed the feat/validate-library-manifest branch from 039a2b0 to 2d8d2cf Compare June 20, 2023 04:11
@y-chan
Copy link
Member Author

y-chan commented Jun 20, 2023 •
edited
Loading

レビューいただいた点を修正しました！

また、vvlib_manifestのバリデーションを少し強化してみました(したほうが楽そうだったので)

Hiroshiba
Hiroshiba approved these changes Jun 20, 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！！！！！！

テスト素晴らしいと思います！！
いくつかコメントしたので気に入ったのあれば 🙏
そのままでも問題ないと思うので、忙しければ仰って頂ければ！

test/test_downloadable_library.py Outdated Show resolved Hide resolved
test/test_downloadable_library.py Outdated Show resolved Hide resolved
test/test_downloadable_library.py Outdated Show resolved Hide resolved
test/test_downloadable_library.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba approved these changes Jun 20, 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！！！

問題ないと思うのでマージします！

@sevenc-nanashi ライブラリ管理機能周りなので、もしなにか気づいたことがあればコメント頂けると心強いです･･･！

@Hiroshiba Hiroshiba merged commit c7e532a into VOICEVOX:master Jun 20, 2023
This was referenced Jun 12, 2023
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@y-chan @Hiroshiba