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

追加: OJT 音素のバリデーション #1004

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jan 9, 2024

内容

OJT 音素のバリデーション追加

VOICEVOX ドメインへ渡されていた xx OJT音素を Label クラスの段階でエラーとした。
また OJT出力そのもののバリデーションをおこなった。
現行の xx に対する振る舞いを記述した(臨時の)異常系テスト test_create_accent_phrases_toward_unknown() を廃止した。

関連 Issue

#958 (comment)
#982

@tarepan tarepan requested a review from a team as a code owner January 9, 2024 18:04
@tarepan tarepan requested review from Hiroshiba and removed request for a team January 9, 2024 18:04
Copy link

github-actions bot commented Jan 9, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 513 277 coverage-46%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core/init.py 0 0 coverage-100%
voicevox_engine/core/core_adapter.py 81 12 coverage-85%
voicevox_engine/core/core_initializer.py 59 30 coverage-49%
voicevox_engine/core/core_wrapper.py 257 183 coverage-29%
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 4 coverage-94%
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 12 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 6 coverage-79%
voicevox_engine/metas/init.py 0 0 coverage-100%
voicevox_engine/model.py 186 9 coverage-95%
voicevox_engine/morphing.py 71 46 coverage-35%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 2 coverage-98%
voicevox_engine/preset/init.py 0 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 17 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 152 6 coverage-96%
voicevox_engine/tts_pipeline/tts_engine.py 267 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_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2475 689 coverage-72%

@tarepan tarepan changed the title 追加: xx OJT 音素のバリデーション 追加: OJT 音素のバリデーション Jan 9, 2024
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.

型周りでいろいろ調べてみたので相談コメントをさせていただきました 🙇
正直Pythonの型システムはイマイチなので、どこまで頑張りましょうという感じです。


Literalの音素型と実体の音素リストがあると、抜け漏れを検知できなかったりと課題がありそうな気がしました!
ということで片方の定義だけで済む方法を探ってたのですが、どーーーーーにもうまい方法が思いつきませんね・・・・・・・。

pydantic v2を使って、型定義だけから実行する方法がこんな感じでした。
結構ややこしくなってしまったのですが、とりあえずここにメモさせていただきます。。

from typing import Literal, cast

from pydantic import TypeAdapter


class OjtUnknownPhonemeError(Exception):
    pass


class NonOjtPhonemeError(Exception):
    pass


Phoneme = Literal["a", "e", "i", "o", "u"]
UnknownPhoneme = Literal["xx"]

_PhonemeAdapter = TypeAdapter(Phoneme)
_UnkownPhonemeAdapter = TypeAdapter(UnknownPhoneme)


def hoge(p: str) -> Phoneme:
    # 便利関数定義すればもうちょっとマシになりそう
    try:
        return cast(Phoneme, _PhonemeAdapter.validate_python(p))
    except:
        try:
            _UnkownPhonemeAdapter.validate_python(p)
        except:
            raise NonOjtPhonemeError()
        raise OjtUnknownPhonemeError()


try:
    print(hoge("a"))
except Exception as e:
    print(type(e))

もう1つ、実体のsetとTypeGuardを使って、Literal型をなくしてみました。こっちはシンプルかも・・・?
(pyrightならたぶんTypeGuardもいらない)

from typing import NewType, TypeGuard


class OjtUnknownPhonemeError(Exception):
    pass


class NonOjtPhonemeError(Exception):
    pass


Phoneme = NewType("Phoneme", str)


phonemes = {"a", "e", "i", "o", "u"}
unknown_phoneme = "xx"


def is_phoneme(p: str) -> TypeGuard[Phoneme]:
    return p in phonemes


def hoge(p: str) -> Phoneme:
    if is_phoneme(p):
        return p
    elif p == unknown_phoneme:
        raise OjtUnknownPhonemeError()
    else:
        raise NonOjtPhonemeError()


try:
    print(hoge("xx"))
except Exception as e:
    print(type(e))

test/tts_pipeline/test_text_analyzer.py Outdated Show resolved Hide resolved
voicevox_engine/tts_pipeline/text_analyzer.py Outdated Show resolved Hide resolved
elif p == "xx":
raise OjtUnknownPhonemeError()
else:
# NOTE: mypy が型推論に失敗。pyright の推論した型が返り値型と一致することをマニュアル確認済み @2024-01-10 tarepan
Copy link
Member

Choose a reason for hiding this comment

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

このNOTEはずっと残ることを想定されていますか? 👀
もしそうであれば、typing.TypeGuardを使うととりあえず外す方法がわかったので、プルリク送ろうかなと考えています。

あるいはtyping.castを使えば、とりあえず# type: ignoreは避けられると思います。
(解決策がだいぶいまいちですが。。)

@@ -47,6 +48,55 @@
]
OjtUnknown = Literal["xx"]
OjtPhoneme = OjtVowel | OjtConsonant | OjtUnknown
_OJT_PHONEMES: list[OjtPhoneme] = [
Copy link
Member

Choose a reason for hiding this comment

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

tupleか、もっと言えばsetのが良さそうかなと!
あとtyping.Final使えば再代入も(静的に)禁止できそうでした。

@tarepan
Copy link
Contributor Author

tarepan commented Jan 17, 2024

Literalの音素型と実体の音素リストがあると、抜け漏れを検知できなかったりと課題がありそう

👍
ご指摘の通りです。

もう1つ、実体のsetとTypeGuardを使って、Literal型をなくしてみました ... pyrightならたぶんTypeGuardもいらない

👍
この方向性に同意です。
議論を「Literal型廃止」と「型推論/TypeGuard」の2段階に分けるとスッキリしそうです。

Literal型廃止

実体を list から tuple (≠ set) へ変更することで、Literal Union が (pyright で) 型推論されることを確認しました。mypiは型推論に失敗します。
type: ignore を利用する現在の PR ではこの改善をエラー無く利用できるため、「Literal型廃止」に同意です。追加 commit をおこないました。

型推論/TypeGuard

TypeGuard を用いた改善に関して、認識の確認をさせてください。
タイプガード関数は is_voicevox_phoneme() を意図している、で認識合っているでしょうか?

@Hiroshiba
Copy link
Member

TypeGuard を用いた改善に関して、認識の確認をさせてください。
タイプガード関数は is_voicevox_phoneme() を意図している、で認識合っているでしょうか?

ちょっとis_voicevox_phoneme()が何を示しているか分からないのですが、タイプガードが必要なところに必要なものを実装していく、みたいな意図でした!

今自分が把握している必要なとこは↓で、is_ojt_phonemesは必須、あと必要ならis_ojt_unknownsとかかなと!
https://github.com/VOICEVOX/voicevox_engine/pull/1004/files#r1446976680

@tarepan tarepan mentioned this pull request Feb 3, 2024
@Hiroshiba
Copy link
Member

あ、こちらのPRどうでしょう? 👀  (なにか返答待ち中でしたらすみません 🙇 )

@tarepan
Copy link
Contributor Author

tarepan commented Feb 20, 2024

(お待たせしました🙇)

現状

(時間が空いたので、現状再確認)

変更点は以下の2つ:

  • A.(VOICEVOX ドメインへ渡されていた)xx OJT音素を (OJT ドメインの)Label クラスの段階でエラー化
  • B. OJT出力そのもののバリデーション追加

Aは問題無し。
Bは型システム上の課題あり。

Bでは、Literal Union narrowing の mypy 非対応を原因とする、type: ignore が課題(pyrightは型推論できる)。
よってタイプガード導入によって type: ignore を無くせるか議論中。

レビュー議論

今自分が把握している必要なとこは↓で、is_ojt_phonemesは必須、あと必要ならis_ojt_unknownsとかかなと!

現在のこのコードを:

    def phoneme(self) -> Vowel | Consonant | Literal["sil"]:
        """このラベルに含まれる音素。子音 or 母音 (無音含む)。"""
        p = self.contexts["p3"]
        if p not in _OJT_PHONEMES:
            raise NonOjtPhonemeError()
        elif p == "xx":
            raise OjtUnknownPhonemeError()
        else:
            return p  # type: ignore

タイプガードで次のように改良する:

    def phoneme(self) -> Vowel | Consonant | Literal["sil"]:
        """このラベルに含まれる音素。子音 or 母音 (無音含む)。"""
        p = self.contexts["p3"]
        if is_ojt_phoneme(p):
            raise NonOjtPhonemeError()
        elif is_ojt_unknown(p):
            raise OjtUnknownPhonemeError()
        else:
            return p

という提案だと理解しました。

この提案は is_ojt_phoneme() 実装上の問題があると思います。
実装は次のようになるかと思います:

is_ojt_phoneme(p: str) -> TypeGuard[OJT_PHONEMES]:
    return p in _OJT_PHONEMES

しかし OJT_PHONEMES 相当の型は上記の「Literal型廃止」議論に基づいて削除されており、この関数が定義できません。
OJT_PHONEMES型を定義すると_OJT_PHONEMES タプルとの二重管理問題が再燃します。

なにか良い解決策あるでしょうか?

@Hiroshiba
Copy link
Member

@tarepan

OJT_PHONEMES型を定義すると_OJT_PHONEMES タプルとの二重管理問題が再燃します。
なにか良い解決策あるでしょうか?

型の方はただのstrのNewTypeにするのを考えていました!

Consonant = NewType("Consonant", str)
Vowel = NewType("Vowel", str)
Phoneme = NewType("Phoneme", str)

vowels: set[Vowel] = {"a", "e", "i"}
consonants: set[Consonant] = {"k", "s"}
unknown_phoneme = "xx"
phonemes: set[Phoneme] = vowels | consonants | {unknown_phoneme}

def is_vowel(p: str) -> TypeGuard[Vowel]:
    return p in vowels

def is_consonant(p: str) -> TypeGuard[Consonant]:
    return p in consonants

def is_phoneme(p: str) -> TypeGuard[Phoneme]:
    return p in phonemes

こうした場合、if elseの順番を考えたりしないといけないので若干厄介ですが、なんだかんだこれがいいのかなと思っています・・・。
Pylanceがmypyくらい簡単にCUIから使えると嬉しいのですが・・・。

@tarepan
Copy link
Contributor Author

tarepan commented Feb 23, 2024

型の方はただのstr

なるほど。
型推論に基づいて OJTPhoeneme - {"xx"} ⊂ VVPhoneme を保証するのではなく、str でいい具合に留める、ということですね。

ちなみに pylance (pyright) 導入はどんな温度感でしょうか?
pyright自体はCLIから普通に呼べるため、タスクランナー周りが解決すれば割と楽に導入できる気はします。
mypy → pyright だと型検査方針が多少なり違うので、それに対する温度感が知りたいです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 23, 2024

ちなみに pylance (pyright) 導入はどんな温度感でしょうか?

型検査部分だけならpylanceのが良さそうなのですが、良い感じのCLIツールが無いことが割りと致命的に感じてます。

たしかGithub ActionsがあるのでCIは簡単に解決すると思います。VSCode使ってる人はストレスは少なめかもです。
ただそれ以外の人にとっては「pushして3分待たないとテストが通るかわからない」ことになりそうなのと、あとVSCode 依存だと他にPylanceが必要なプロジェクトがあった場合にバージョン問題(要求されるバージョンが異なる)が発生して大変そうだなと・・・。

mypyもだいぶ辛いですが、pylanceも別の辛さがあって、さてどっちのがマシかなって感じです 😇
1回導入しても良い・・・かも・・・?でもどうせ半年後くらいにバージョン問題かCLI問題が顕在化して辛くなってくるんですよね・・・。
pylanceプラグインが使えないコードエディタを使ってる人はほぼ居ないと考えて、まあ CLI は置いといてバージョンの方が解決する(例えばpyprojectに書けるとかVSCodeプラグイン内でバージョン指定できるとか)なら導入もありかも・・・。

みたいな温度感です!

(大きめのOSSでの導入事例があると真似すればいいかも。)

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