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

AquesTalk記法のバリデータ (#315) #681

Merged
merged 6 commits into from
May 25, 2023
Merged

AquesTalk記法のバリデータ (#315) #681

merged 6 commits into from
May 25, 2023

Conversation

FujisakiEx
Copy link
Contributor

内容

AquesTalk記法のチェックを行う正規表現とそのテストです
issue と kana_parser.py を見る限りでは　本家AquesTalkに従わなくてもいいのかなと思い、現状生成されうるものに従ったテストにしています

関連 Issue

ref #315
ref #316

その他

ref #316の議論がそもそも尽くされていなさそうなのと、初のPRで色々勝手がわかっていないのでAPIを用意するところまではやっていません。

@FujisakiEx
Copy link
Contributor Author

pre-commitのインストールが抜けていました
指摘を反映しました

@Hiroshiba
Copy link
Member

PRありがとうございます！

良いですね！！
思いつく限りのコーナーケースのテストを書きたいですね！
空欄とか、もっとアクセント区の数が多いのとか。

validate関数を作って実際にparse_kana辺りからバリデーションしますか。
（そうしないと現状なんのテストかわからないかもなので）

その辺できたらdraft外しちゃっていただけると！

@github-actions
Copy link

github-actions bot commented May 9, 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/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 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 1413 284 coverage-80%

FujisakiEx
FujisakiEx commented May 13, 2023
View reviewed changes
test/test_kana_parser.py Outdated
@@ -535,6 +536,7 @@ def _assert_error_code(self, kana: str, code: ParseKanaErrorCode):
parse_kana(kana)
self.assertEqual(err.exception.errcode, code)

@unittest.skip
Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_kanaの先頭でvalidate_kanaにより非受理の判定だった時点で既存の処理をスキップするようにしたため、いったんテストを無効化しています

FujisakiEx
FujisakiEx commented May 13, 2023
View reviewed changes
voicevox_engine/kana_parser.py Outdated
@@ -89,6 +104,9 @@ def parse_kana(text: str) -> List[AccentPhrase]:
if len(text) == 0:
raise ParseKanaError(ParseKanaErrorCode.EMPTY_PHRASE, position=1)

if not validate_kana(text):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validate_kanaは非受理の原因までは判別しないため、エラーコードは適当なものになっています

@FujisakiEx
Copy link
Contributor Author

parse_kanaのエラーコード処理の部分が完全に頭から抜けていました
文字列が非受理だった場合、非受理の原因まで判定してエラーコードを投げるように変更し、既存のコードとテストが動作するようにしたほうがいいでしょうか？

@Hiroshiba
Copy link
Member

Hiroshiba commented May 16, 2023 •
edited
Loading

@FujisakiEx たしかにエラー内容がわからないと不便ですね･･･！！
今思えば、parse_kana関数はすでにバリデーション機能がある＆処理も遅くないので、validate_kana関数を追加せず、もうparse_kanaを実行てparseできたらvalid、できなかったらinvalidとする手もありかも？とちょっと思いました･･･！

@FujisakiEx
Copy link
Contributor Author

@Hiroshiba 了解しました、私もそれで問題ないかと思います
PRのほうは閉じておきます、元issueのほうはお任せします

@FujisakiEx FujisakiEx closed this May 20, 2023
@Hiroshiba
Copy link
Member

@FujisakiEx 元issueはバリデーション用の新しいAPIを作るかどうかということなので、まだ未解決なので残しておくべきかなと思います。
正しいAPIを作るというのがもともとの目標だったと思うので、実際にそのAPIを作ってみるとかどうでしょう？
議論を重ねることで、この辺に詳しくなったと思うので @FujisakiEx さんが適任なのかなと思いました…！

@FujisakiEx
Copy link
Contributor Author

@Hiroshiba　すいません、私の考えているAPIの理解が間違っていました
このまま担当させていただきます

@FujisakiEx FujisakiEx reopened this May 20, 2023
@Hiroshiba
Copy link
Member

あ！　たしかにissueにちゃんと書いてなかったです、web APIのことを意図していました！
issue側に追記しておこうと思います。

FujisakiEx
FujisakiEx commented May 20, 2023
View reviewed changes
@Hiroshiba
Copy link
Member

Hiroshiba commented May 20, 2023 •
edited
Loading

レビューしても問題なさそうになったらdraftを外して頂ければ！

@FujisakiEx FujisakiEx marked this pull request as ready for review May 20, 2023 19:38
@FujisakiEx FujisakiEx requested a review from a team as a code owner May 20, 2023 19:38
@FujisakiEx FujisakiEx requested review from Hiroshiba and removed request for a team May 20, 2023 19:38
Hiroshiba
Hiroshiba reviewed May 22, 2023
View reviewed changes
run.py Outdated
)
def validate_kana(text: str):
"""
テキストがAquesTalk記法に従っているかどうかを判定します.
Copy link
Member

Choose a reason for hiding this comment

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

他のとこと名称などを合わせて「AquesTalkライクな記法」、と句点は「。」でお願いします！
あとエラーを返すことも伝えると丁寧かもです。こんな感じとか･･･？

Suggested change
テキストがAquesTalk記法に従っているかどうかを判定します.
テキストがAquesTalkライクな記法に従っているかどうかを判定します。
従っていない場合はエラーが返ります。

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

@takana-v さんも見て頂けると心強いです！！

takana-v
takana-v approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@takana-v takana-v left a comment

Choose a reason for hiding this comment

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

LGTMです！

@FujisakiEx
Copy link
Contributor Author

フィードバックを反映しました、コミットログが5つに分かれており、最終的に消える変更ばかりなのでrebaseしたほうがよいでしょうか？

@takana-v
Copy link
Member

「Squash and marge」でマージするので大丈夫です！
一か所Suggestionの適用忘れと思われる個所があったので、私の方で変更を取り込んでマージしたいと思います。

takana-v
takana-v reviewed May 25, 2023
View reviewed changes
@takana-v
Copy link
Member

テストが通ったのでマージします。
APIの追加、ありがとうございました！

@takana-v takana-v merged commit c341eb8 into VOICEVOX:master May 25, 2023
@FujisakiEx
Copy link
Contributor Author

@takana-v
mergeありがとうございました。修正もありがとうございます

@Hiroshiba
Copy link
Member

は閉じちゃって良さそうなので閉じます･･･！
またのプルリクエストをお待ちしております！！！

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takana-v takana-v takana-v approved these changes

@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.
3 participants
@FujisakiEx @Hiroshiba @takana-v