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!: AudioQueryのJSON表現をENGINEと同じにする #946

Merged
merged 11 commits into from
Feb 3, 2025

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 25, 2025

内容

#261 が行われた理由は次の2点である。

  1. jsonフィールドのcaseを揃えたほうが良いのではないか voicevox_engine#460 もやろうとしていた
  2. (少なくとも正攻法では)pydantic.dataclassesの対応ができない (PyO3を使ってクレートからwhlを作る #239 (comment))

1.は中止、2.もこの度 ダーティハックであるがなんとかする方法を発見したため、 pydantic.TypeAdapterを利用および推奨すればよいことがわかったため、 #261 の取り消しを行う。これでENGINEとCOREでAudioQueryを使い回せるようになる。

BREAKING-CHANGE: AudioQueryのJSON表現がENGINEと同じになる。
See-also: https://docs.python.org/3.13/library/dataclasses.html#dataclasses.Field
See-also: https://docs.pydantic.dev/2.10/api/type_adapter/

関連 Issue

その他

@qryxip qryxip changed the title feat!: ちょっとしたダーティハックを行うことで #261 を差し戻し feat!: ちょっとしたダーティハックを行うことで #261 を取り消し Jan 25, 2025
@qryxip qryxip requested a review from Hiroshiba January 25, 2025 20:14
@qryxip qryxip force-pushed the feat-revert-pr261-with-a-dirty-hack branch from b044c07 to 04dadbd Compare January 25, 2025 20:23
Comment on lines 198 to 200
@pydantic.dataclasses.dataclass(
config=ConfigDict(alias_generator=_rename_audio_query_field),
)
Copy link
Member Author

@qryxip qryxip Jan 25, 2025

Choose a reason for hiding this comment

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

Alias - Pydantic

なんか幸いなことに、特に設定しない場合の挙動はエイリアスというよりはリネームっぽい。snake_caseなやつを受け付けなくなった。
(ただしこれはdataclassとしてのフィールド名には影響しないので、そっちの対処はこの下の__post_init__で行う。)

@@ -231,6 +242,19 @@ class AudioQuery:
のAudioQueryでは無視される。
"""

# `dataclasses.asdict`の内部実装に依存したハックだが、他に方法が思い付かなかった。
Copy link
Member Author

@qryxip qryxip Jan 25, 2025

Choose a reason for hiding this comment

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

よく考えてみれば別にそんなにハックじゃないような気がしてきた。問題はdataclasses.Field.nameを外から書き換えていいのかといったところか。駄目とは書かれていない。

qryxip added a commit to qryxip/voicevox_core that referenced this pull request Jan 25, 2025
@qryxip qryxip force-pushed the feat-revert-pr261-with-a-dirty-hack branch from 9a0fe3a to 31b8dea Compare January 25, 2025 21:13
Comment on lines 261 to 264
if true_name := self.__attr_true_names.get(name):
return getattr(self, true_name)
# 普通の`AttributeError`と同じ文面
raise AttributeError(f"{type(self).__name__!r} has no attribute {name!r}")
Copy link
Member Author

Choose a reason for hiding this comment

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

多分こういう書き方もあるが、どっちが良いかどうか正直わからない。

Suggested change
if true_name := self.__attr_true_names.get(name):
return getattr(self, true_name)
# 普通の`AttributeError`と同じ文面
raise AttributeError(f"{type(self).__name__!r} has no attribute {name!r}")
try:
true_name = self.__attr_true_names[name]
except KeyError:
# 普通の`AttributeError`と同じ文面
raise AttributeError(f"{type(self).__name__!r} has no attribute {name!r}")
return getattr(self, true_name)

Comment on lines 246 to 259
def __post_init__(self) -> None:
"""
:func:`dataclasses.asdict` にてキーが正しい名前になるよう、 ``dataclass``
としてのフィールドをハックする。
"""

self.__attr_true_names: dict[str, str] = {}
for field in dataclasses.fields(self):
if (rename := _rename_audio_query_field(field.name)) != field.name:
self.__attr_true_names[rename] = field.name
field.name = rename

def __getattr__(self, name: str) -> object:
"""camelCaseの名前に対し、対応するsnake_caseの名前があるならそれについて返す。"""
Copy link
Member Author

@qryxip qryxip Jan 26, 2025

Choose a reason for hiding this comment

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

この二つをSphinxがそのまま出すため、docstringをちゃんと書いたという流れ。なのでこれらはユーザー向けの説明である。

image

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.

すみません、確認です!

AudioQuery jsonとコード内でのデータ形式どちらともキャメルケースとスネークケースが入り混じる形になるようになった、という感じでしょうか?(違いそう)

そもそもエンジンと同じにすべきなのかを少し迷っています。同じにした方が良い気もしつつ、スネークケース統一だった方が良い気もしつつ………難しいですね。。。

@qryxip
Copy link
Member Author

qryxip commented Jan 26, 2025

あ、コード内はsnake_caseのまま、JSON表現だけをENGINEと同じにしています。

よく考えたらPRのタイトルはよくありませんね。変えておきます。

@qryxip qryxip changed the title feat!: ちょっとしたダーティハックを行うことで #261 を取り消し feat!: AudioQueryのJSON表現をENGINEと同じにする Jan 26, 2025
@Hiroshiba
Copy link
Member

なるほどです!!

あとはこの点かなぁ

そもそもエンジンと同じにすべきなのかを少し迷っています。同じにした方が良い気もしつつ、スネークケース統一だった方が良い気もしつつ………難しいですね。。。

うーーーーーん。
少なくともエンジン側のAPIの互換性は、データ構造置き換えれば良いだけなので考慮しなくても良さそう。

エンジンとAudioQueryのデータ構造を合わせるべきか、ということだよなぁ。
合わせるとどんな感じで良さそうでしょうか?👀
(合わせたくないのではなく、合わせた方が良いのかどっちかわからないという視点です)

@qryxip
Copy link
Member Author

qryxip commented Jan 26, 2025

#943 (comment)でも書いたのですが、ENGINEとCOREを両方使うケースはあるかなと思ってます。両者を比較検討しているときとか。またユーザーが見るのがドキュメントではなくZennやChatGPTに寄るということも起こりうることを考えると、「ENGINEを使う知識」と「COREを使う知識」の違いは、可能な範囲において小さくした方がよいのではないかと思った次第です。

@qryxip qryxip requested a review from Hiroshiba January 26, 2025 16:09
@Hiroshiba
Copy link
Member

ENGINEとCOREを両方使うケースはあるかなと思ってます

個人的にはこれはかなりレアケースかなと思ってます!
自分の目的に合う段階で片方になることがとても多いのと、あとそれに比べて圧倒的に片方だけ利用する人が多いかなと。

ユーザーが見るのがドキュメントではなくZennやChatGPTに寄るということも起こりうること

こちらは同感です!

となると、シリアライズされるものというか、エンジンが出力するjsonが既存である場合は合わせる形が良さそう。
多分このプルリクエストはその形になっているので、良さそう!!

@qryxip qryxip requested a review from Hiroshiba January 28, 2025 09:38
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!!

2点だけコメントしてます!

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.

JSON化(シリアライズ化)するためにdataclass.asdictを使っていますが、ここがユーザーに露出するとdataclass(のasdict)に依存したAPIになってしまうので、数年後に不便になるだろうなと感じました!

たぶん良いのはserializeメソッドを作るか、あるいはserialize関数を別で実装するかだろうなと思いました。
たぶんjsonという文字列も出さない方が良さそう。(jsonをサポートしてるわけではなく、僕たちにとって都合のいいのがたまたまjsonだったというスタンス)

けどまあこうするとRust側のAPIと乖離してしまうし、ちょっと考えないといけないこと多いので大変な気もします。
とりあえず今はasdictで実装し、APIとしてはサポートせず(ドキュメントに書かず)、後回しにするとかもありだと思います!

@qryxip
Copy link
Member Author

qryxip commented Jan 30, 2025

5f8e4aa (#946)

ちょっと調べたのですが、PydanticのdataclassからJSONを吐き出す経路としてはpydantic.TypeAdapter.dump_jsonがあることがわかりました。こっちの経路ではfieldsの書き換えは効果は発揮せず、必ず呼ぶたびにby_alias=Trueを付けなければなりません。dump_pythonjson_schemaというのもありますが、これらも同様です。by_alias=Trueを付けないとsnake_caseのJSON Schemaをお出ししてきます。クラス自体について設定する項目は見つかりませんでした。

# snake_caseのまま吐き出されてしまう!(当然、JSON → dataclass → JSONと変換したら元のJSONから変化してしまう)
_ = TypeAdapter(AudioQuery).dump_json(query).decode()

# `by_alias`(デフォルトは`False`)を`True`にするとちゃんとcamelCaseで出る
_ = TypeAdapter(AudioQuery).dump_json(query, by_alias=True).decode()

これを受け、最早asdictのサポートはしなくていいなと思ったため、関連のハックを取り消した上でTypeAdapterを使う方法に切り替えてドキュメントの注意書きも書きました。お手数をおかけするのですが、もう一度レビューをお願いできればと…

@qryxip qryxip requested a review from Hiroshiba January 30, 2025 12:47
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!!

ちなみに目標とする0.16リリースの機能の中にシリアライズは含まれていないので、本線進めた方が良いのではとかちょっと思いました・・・!!

あとこれはただの共有ですが、今のバージョンでシリアライズしたものが読み込めることを今後ずっと確認するテストもあると嬉しいかもですね!
まあデータ構造は変わったタイミングで良いはず・・・!

@qryxip
Copy link
Member Author

qryxip commented Feb 3, 2025

とりあえずapprove頂いてたのでマージ!

@qryxip qryxip merged commit d810e4b into VOICEVOX:main Feb 3, 2025
36 checks passed
@qryxip qryxip deleted the feat-revert-pr261-with-a-dirty-hack branch February 3, 2025 19:41
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