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

feat!: [Python] EnumLiteral #950

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 26, 2025

内容

目的としては二つ。一つは VOICEVOX/voicevox_engine#1413 と同じで、書く人、今回の場合つまりユーザーの利便性を高めるため。もう一つはこの後別PRで | strを付けてLiteral[…] | str Literal[…] | _Reserved とすることで、Rustで言うnon exhaustiveとし、後方互換性を確保できるようにするため。

関連 Issue

See-also: #941 (comment)

その他

@qryxip qryxip requested a review from Hiroshiba January 26, 2025 17:52
@qryxip qryxip force-pushed the pr/feat-python-enum-to-literal branch from 61507ca to 5a70552 Compare January 26, 2025 17:54
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.

typescriptのenumはともかく、PythonのEnumは実は問題ないかも?とか少し思いました!
Literal stringは型の意味しかないですが、Enumクラス自体は値なのでどっちかというとこっちのが便利ではありそう。

例えばexample/python/run.pyはたぶんchoicesを使ってこうも書けます:

argparser.add_argument(
    "--mode",
    default="AUTO",
    type=StyleType,
    choices=StyleType.__members__.values()
    help='モード',
)

あとバリデーションもこれで可能です:

style: str = "hoge"
style_ = StyleType(style)
# ValueError: 'hoge' is not a valid StyleType

内部で雑に使うliteralだったらliteral stringで良さそうだけど、公開API用だとEnumのが良いかも・・・?
まあでもそこまで強い意見じゃないです!

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented Jan 26, 2025

公開API用だとEnumのが良いかも・・・?

前方互換性のためみたいな感じだったような?
具体的には:
Enumだと

from enum import Enum

class Color(Enum):
    RED = 1
    GREEN = 2
    BLUE = 3

みたいだったときに

if color == Color.RED:
    ...
elif color == Color.GREEN:
    ...
else: # color == Color.BLUE:
    ...

と書く人が多く、それだと新しく何かを追加したときに既存のコードが壊れて互換性破壊になります。

そこで「今後増える可能性がある」というのを明示するために「一応パターンは決まってるけど必ずしも全部じゃ無いよ(だから必ずelse節を作ってね、など)」というのを表したく、そのためにTSでいう"red" | "green" | "blue" | stringにする

...というのが #941 の意図という認識です。

@Hiroshiba
Copy link
Member

なるほどです!! non_exhaustiveを理解しきれてませんでした・・・!

一応Enumでも同じことはできる・・・けど、なんか微妙に複雑になりそうですねぇ・・・

class StyleType(str, Enum):  # こっちがユーザー用
  ...

NonExhaustiveStyleType = StyleType | str  # こっちはAPI用

あと| strを付けると次はタイポに気づけなくなるのでLiteral Stringの恩恵がなくなりそう。

StyleType: TypeAlias = Literal["talk", "sing"] | str

my_style: StyleType = "song"  # singの間違い

Literal["non_exhaustive"]を足すとかStrEnumにnon_exhaustiveを足すとかもありかもだけどこれはこれで妙なことになりそう・・・。

直感、言語的なサポートなくnon_exhaustiveっぽいのを実装するのは課題も多そうに感じました!
意識する人は型サポートがなくても意識すると思うし、意識しない人は型に| strがあっても「なんでこんな仕様なんだ」ってなるだけな気がするので、ドキュメントで案内するに留めるのが良いのかもなぁと思いました。

一旦| strにするのはタイポに気付けないのでちょっと反対寄り・・・・・・かなぁ。。。
ちょっと考えきれてないです🙇

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2025

あと| strを付けると次はタイポに気づけなくなるのでLiteral Stringの恩恵がなくなりそう。

| NonExhaustiveみたいなのはどうでしょうか?NonExhaustiveはコンストラクタ不可の実質的なボトム型で、Pydantic側から問い合わせが来ても全拒否するようにします。
(「実質的な」というのは、ボトム型であることがPyright/Pylanceやmypyにバレたら意味が無くなってしまうため)

[追記] イメージはこんな感じ。

from enum import Enum
from typing import Any, Literal, NoReturn, TypeAlias

from pydantic import GetCoreSchemaHandler
from pydantic_core import CoreSchema, core_schema


class NonExhaustive(Enum):
    pass

    # 以下の二つは無くてもいいけど、少しでもましなエラーメッセージのため

    @classmethod
    def __get_pydantic_core_schema__(
        cls, source_type: Any, handler: GetCoreSchemaHandler
    ) -> CoreSchema:
        # TODO: pydantic/pydantic-core#1579 の機能がリリースされたらそれを使う
        return core_schema.no_info_after_validator_function(
            cls._no_input_allowed, core_schema.any_schema()
        )

    @classmethod
    def _no_input_allowed(cls, _: object) -> NoReturn:
        raise ValueError(f"No input is allowed for `{cls.__name__}`")


StyleType: TypeAlias = (
    Literal["TALK", "SINGING_TEACHER", "FRAME_DECODE", "SING"] | NonExhaustive
)

例外メッセージはこう。 pydantic/pydantic-core#1579 を見るにこれが限度っぽい。

Traceback (most recent call last):
  File "/home/ryo/src/github.com/VOICEVOX/voicevox_core/main/crates/voicevox_core_python_api/a.py", line 35, in <module>
    x = StyleMeta(style_type="間違った文字列")
  File "/home/ryo/.cache/pypoetry/virtualenvs/voicevox-core-hMLxFPJX-py3.10/lib/python3.10/site-packages/pydantic/_internal/_dataclasses.py", line 141, in __init__
    s.__pydantic_validator__.validate_python(ArgsKwargs(args, kwargs), self_instance=s)
pydantic_core._pydantic_core.ValidationError: 2 validation errors for StyleMeta
style_type.literal['TALK','SINGING_TEACHER','FRAME_DECODE','SING']
  Input should be 'TALK', 'SINGING_TEACHER', 'FRAME_DECODE' or 'SING' [type=literal_error, input_value='間違った文字列', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/literal_error
style_type.function-after[_no_input_allowed(), any]
  Value error, No input is allowed for `NonExhaustive` [type=value_error, input_value='間違った文字列', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/value_error

一応Javaではこういう方法がいいと考えています。

public class StyleType {
  public static final StyleType TALK = new StyleType();
  public static final StyleType SINGING_TEACHER = new StyleType();
  public static final StyleType FRAME_DECODE = new StyleType();
  public static final StyleType SING = new StyleType();

  private StyleType() {}
}

ただPythonの方は、ここ↓まではいけてJSON → dataclass (via Pydantic)はできるけど、dataclass → JSONが詰んでしまうと思っています。何故なら純粋なdataclassにPydanticが介入できるポイントは無いので。
(#946 の手法も使えない)

# Rust (PyO3)によるオブジェクト
class StyleType:
    # 純Pythonだとここで詰むけど、PyO3ならいける…はず?
    TALK: Final["StyleType"]
    SINGING_TEACHER: Final["StyleType"]
    FRAME_DECODE: Final["StyleType"]
    SING: Final["StyleType"]

    # Pydanticのため
    def __new__(cls, s: str) -> "StyleType": ...

    # 「この型のJSON表現はstringですよ」ということをPydanticに伝える
    @classmethod
    def __get_pydantic_core_schema__(cls, source_type: Any, handler: GetCoreSchemaHandler) -> CoreSchema: ...

@qryxip qryxip requested a review from Hiroshiba January 27, 2025 03:32
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.

ちょっと事例がないか調べた感じ、typescriptライブラリで| stringが削除されてる例を見かけたのでメモ。削除する流れのが良さそう。
aws/aws-sdk-js-v3#4278


EnumからLiteralにする場合ですが、ドキュメントから削除されたりしませんか 👀
どういうtypeがあり得るのかドキュメント内でわかるようになってたのがわからなくなるなら、Enumに軍配が上がるかもです。

ドキュメントにはEnumであれLiteralであれ書かれない、もしくはどっちでも書かれるなら、まあLiteralで良さそう!
便利なのはクラスが実態として存在するEnumの方だけど(便利なメソッドがあるので)、定義の面倒くささとか考えるとまあLiteralでも。

NonExhaustiveの定義どれが良いかはちょっとわかんないですが、strじゃなければあとはそこそこなんでも良さそう!


ちょっと話し反れるのですが、NonExhaustiveなものとそうじゃない型どっちも必要かも・・・?とか少し思いました。
特に手堅いコードを書きたいわけじゃく、サクッと動かしたい人向けに・・・。
まあでもメンテ大変だし、Rustと合わせる形で良いかも!

@qryxip
Copy link
Member Author

qryxip commented Jan 28, 2025

713fee6 (#950): 忘れないうちに取り急ぎ、 #952 のやつを入れました。

@qryxip
Copy link
Member Author

qryxip commented Jan 28, 2025

ちょっと話し反れるのですが、NonExhaustiveなものとそうじゃない型どっちも必要かも・・・?とか少し思いました。

うーん。そもそも列挙型/直和型を何でもnon exhaustiveにすべきかというと、多分そうではない、というのはありますね。

状況によっては「どう取り繕おうがバリアントを増やした時点で破壊的変更」と言えてしまうこともあるとは思います。例えばあるAPIが以下のようなOutcomeという直和型を返すとします。

#[non_exhaustive]
pub enum Outcome {
    Completed,
    Failed,
    Canceled,
}

これを使うユーザーは、こういうコードしか書けないはずです。こういうケースにおいてはしっかりとexhaustiveとし、バリアントを追加するときになったら潔く破壊的変更とする方が皆が幸福になると思います。

match library::api() {
    Outcome::Completed => info!("OK!"),
    Outcome::Faield => return Err(/* … */),
    Outcome::Aborted => { /* retry */ }
    outcome => {
        panic!("unexpected result. what happened?????: {outcome:?}")
    }
}

で、このPRで対象にしているような列挙型は現在3つあります。
(Rust APIにだけenum voicevox_core::ErrorKindもあって、そっちも#[non_exhaustive]にするべきだとは思っていますが、一旦議論から外します)

  1. AccelerationMode = "AUTO" | "CPU" | "GPU"
  2. UserDictWordType = "PROPER_NOUN" | "COMMON_NOUN" | "VERB" | "ADJECTIVE" | "SUFFIX"
  3. StyleType = "talk" | "singing_teacher" | "frame_decode" | "sing"

1.のAccelerationModeについては、基本的にユーザーからライブラリに渡されるものです。ライブラリからユーザーに渡されるものとしてはAccelerationMode::defaultがありますが、それがAUTOであるという保証を崩さなければ、バリアントを増やしても破壊的変更にはなりえないんじゃないかと思っています。

2.のUserDictWordTypeについても、まあUserDictWordが受理する対象が増えるということで「入力」側とみなしてもいいんじゃないかと思ってます。

問題は3.のStyleTypeで、これが「バリアントを増やした時点で破壊的変更」と言えるかどうか、ちょっとわからないでいます。ユーザーが以下のようなコードを書きたくなる可能性が僅かにでもあるのならyesなんですが…

# 多分CとC++以外、列挙型の網羅性チェックはほぼあらゆる言語に備わっている
match style.type:
    case "talk":
        ...
    case "singing_teacher":
        ...
    case "frame_decode":
        ...
    case "sing":
        ...

特に手堅いコードを書きたいわけじゃく、サクッと動かしたい人向けに・・・。

すみません、こっちはちょっとあまりイメージできませんでした。

…関係あるかはわからないのですが、NonExhaustivestrのサブタイプである必要はありますね。strとして扱えなくなってしまう。

[追記] あと今気付いたけど、NonExhaustive_NonExhaustiveのようにプライベートにしておいてユーザーから触れなくなる必要もありそう(APIドキュメントではリンクされず、正体不明の型と化す)。
というのも、assert not isinstance(x, NonExhaustive), "`NonExhaustive` should be a buttom type!"みたいな無法をされることを想像してしまったので…

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 28, 2025

破壊的変更の考え方を調べてました。
electronの考え方が参考になるかも。 https://www.electronjs.org/ja/docs/latest/breaking-changes

破壊的変更の種別
このドキュメントでは、以下の規約によって破壊的な変更を分類しています。
API 変更: 古いコードで例外の発生が保証されるように API が変更されました。
動作変更: Electron の動作が変更されましたが、例外が必ず発生する訳ではありません。
省略値変更: 古い省略値に依存するコードは動かなくなるかもしれませんが、必ずしも例外は発生しません。 値を明示することで以前の動作に戻すことができます。
非推奨: API は非推奨になりました。 この API は引き続き機能しますが、非推奨の警告を発し、将来のリリースで削除されます。
削除: API または機能が削除され、Electron でサポートされなくなりました。

Outcomeの例は動作変更に当たると思います。

あとGitHub Enterprise Cloud REST APIも参考になりそうでした。
https://docs.github.com/ja/enterprise-cloud@latest/rest/about-the-rest-api/breaking-changes

すべての破壊的変更は、新しい API バージョンでリリースされます。 破壊的変更とは、統合を破損する可能性のある変更のことです。 破壊的変更には次のようなものが含まれます。

  • 操作全体の削除
  • パラメーターの削除または名前変更
  • 応答フィールドの削除または名前変更
  • 新しい必須パラメーターの追加
  • 以前に省略可能だったパラメーターを必須にする
  • パラメーターまたは応答フィールドの型の変更
  • 列挙型の値の削除
  • 既存のパラメーターへの新しい検証規則の追加
  • 認証または認可の要件の変更

これに従うとenumの値を追加するだけなら破壊的変更にならないかもです。


non_exhaustiveにすべきかと、破壊的変更として扱うべきかは別な気がします。
入力側のAccelerationModeUserDictWordTypenon_exhaustiveじゃなくて良さそうだし、仰るとおり破壊的変更にならなそう。
StyleType側は返り値だけど、値を追加しても既存の動作を変更しているわけではないので、破壊的変更じゃなさそう。ただnon_exhaustiveは書いても良さそう。

まとめると、既存の処理が変わらなければ破壊的変更として扱わない、enumの返り値は「それを無視している」コードが正しく動くなら破壊的変更ではない、non_exhaustiveをつけるかはどっちでも、って感じな気がしました!

特に手堅いコードを書きたいわけじゃく、サクッと動かしたい人向けに・・・。

すみません、こっちはちょっとあまりイメージできませんでした。

ちゃちゃっと試しに1回利用するだけのつもりなら「将来値が変わりうる」はどうでもよく、煩わしいこともあるだろうなというニュアンスでした!

@qryxip
Copy link
Member Author

qryxip commented Jan 28, 2025

入力側のAccelerationModeUserDictWordTypenon_exhaustiveじゃなくて良さそうだし、仰るとおり破壊的変更にならなそう。
StyleType側は返り値だけど、値を追加しても既存の動作を変更しているわけではないので、破壊的変更じゃなさそう。ただnon_exhaustiveは書いても良さそう。

あ、逆で、AccelerationModeの網羅性なんかほぼほぼ誰も要らないだろうし、そんな余計な性質の保証をしてしまう前にnon exhaustiveにして塞いでしまうのはどうか?という意味でした!一方StyleTypeにバリアントを追加することがもし「動作変更」にあたるのならnon_exhaustiveせずに網羅性を保証してしまうことを考えていました(先述のOutcomeと同じ理由で)。

ちゃちゃっと試しに1回利用するだけのつもりなら「将来値が変わりうる」はどうでもよく、煩わしいこともあるだろうなというニュアンスでした!

あ、まだちょっとわかっていなくて、返り値の型に| NonExhaustiveが付くことにより損われる利便性って、網羅性が欲しい場合以外に何があるのかなと…

example/python/run-asyncio.py Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 28, 2025

あ、逆で、AccelerationModeの網羅性なんかほぼほぼ誰も要らないだろうし、そんな余計な性質の保証をしてしまう前にnon exhaustiveにして塞いでしまうのはどうか?という意味でした!

ちょっとわかってないですけど、これら2つにnon exhaustiveを付けるのは別に良いと思います!
破壊的変更かどうかとnon exhaustiveかどうかは相関がないはず(=non exhaustiveを付けても付けなくてもどっちでもよく)。

あとはqryxipさんのコメントを見て考えが変わりました。
真にその値がnon exhaustiveならその属性をつければ良いかなと!

一方StyleTypeにバリアントを追加することがもし「動作変更」にあたるのなら、non_exhaustiveにせずに網羅性を保証してしまうことを考えていました(先述のOutcomeと同じ理由で)。

StyleTypeに値を追加することは破壊的変更には当たらない(API側の既存の挙動を変更しない)と思うので、StyleTypeの性質がnon_exhaustiveならnon_exhaustiveをつけるべきかなと。
値は将来増えうるので、non_exhaustiveで良い気がします!

あ、まだちょっとわかっていなくて、返り値の型に| NonExhaustiveが付くことにより損われる利便性って、網羅性が欲しい場合以外に何があるのかなと…

まあこれ結論として、non_exhaustiveはつけても OK だと思います。
non exhaustiveだとnon exhaustiveを意識したコードを書く必要があるので、型を全く意識しない書き捨てコードを書く人にとっては煩わしいかも、というだけです。
個人的には、これはそうかもだけど、APIを長く使う人に合わせてnon exhaustiveをつけた方が良いかなーと思います!

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 28, 2025

再度リテラル文字列と文字列Enum、どちらの方が良いか考えました。
どちらかというとリテラル文字列の方が良さそう!
さらに言うとただの文字列の方がいいだろうなと思いました!これは代わりにexampleを網羅的に用意して代替して解決するのが良さそう!

まず今更気づいたのですが、初学者にとって型はぶっちゃけ理解できないと思います。
int/float/strくらいはわかるけど、それ以上の分解能のものはわからない。(触れたことがない)

StrEnumはクラスなので「インポートしないといけない」という発想にたどり着かない。逆に言うと1回経験すればわかる。どこからインポートすればいいかはわかんないかもだけど。
Literal文字列は型なので「この型に従う値を渡さないといけない」というのが逆にたどり着けない。たぶんどうにかしてLiteral["CPU"]とかを投げようとしてエラーが出る。そして「型は値として渡せません」という全く解決策のわからないエラーが出て詰む。とはいえこちらも1回経験すればLiteralはいらないんだなと気づける。

あともう一度思い出すとawaitも分からないはず。全体的に、一番最初にQiita/Note記事を書こうと頑張る初学者がくじけるレベルの、かなり難しいコードになってそう。
str | Literal["hoge"]を受け入れてバリデーションする形か、str | StrEnumを受け入れる形にして同じくバリデーションするかすると分かりやすそう。
Literal/StrEnumどちらでも、str | になってない型をエクスポートしてあげれば型の恩恵が受けられるはず。

まあでも全体的に優しくすべきというかなり抜本的な話になるので、全体の方針考えないといけず大変なっちゃう。
バージョン0.16で僕たちができるのはとにかくなるべく全部の関数で例を出してあげることかなと。
そしてEnumよりリテラルのほうが簡単(importが不要なので)。

ということでまとめると、とりあえずLiteral文字列の方がよく、あとexampleをなるべく網羅的に用意してあげるのがいいのかなと思いました!

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 requested a review from Copilot January 28, 2025 17:11
Refs: VOICEVOX#950 (comment)

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
qryxip added a commit that referenced this pull request Jan 29, 2025
#941 の続き。また列挙型の網羅性についての議論は以下。
#950 (comment)
@qryxip qryxip merged commit 2410043 into VOICEVOX:main Jan 29, 2025
30 checks passed
@qryxip qryxip deleted the pr/feat-python-enum-to-literal branch January 29, 2025 16:36
qryxip added a commit that referenced this pull request Jan 30, 2025
#941, #955, #950 の続き。

`_Reserved`は`voicevox_core._models._please_do_not_use._Reserved`として
でしか存在せず、ユーザーは原則触ることはできない状態にしてある。

BREAKING-CHANGE: Rust API, Java APIと同様`AccelerationMode`, `StyleType`, `UserDictWordType`がnon exhaustiveに。
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.

3 participants