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

FIX: AudioQueryの互換性の問題を修正 #1425

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

sabonerune
Copy link
Contributor

内容

AudioQueryに追加されたpauseLengthScaleが必須になっているため過去のクエリを保存していたり古いOpenAPIに基づいて生成したクライアントがモデル内で宣言していないキーを削除していた場合互換性の問題が発生します。

pauseLengthScaleにデフォルト値を設定することで互換性を維持します。

関連 Issue

その他

とりあえずエディタで動作することは確認しました。
一応テストも通ったので問題はないと思いますがこの辺りの変更に詳しくないので確認不足があるかもしれません。

また、テストも追加するべきかもしれません。

@sabonerune sabonerune requested a review from a team as a code owner June 24, 2024 11:54
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team June 24, 2024 11: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.

LGTM!!

ありがとうございます、助かります!! 🙇
別のタスクがつかえてしまうので急ぎめでマージします!

Comment on lines 28 to 30
pauseLength: float | SkipJsonSchema[None] = Field(
default=None, title="句読点などの無音時間"
)
Copy link
Member

Choose a reason for hiding this comment

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

ついでにすみません!!

SkipJsonSchema[None]って、Noneが受け入れ不可なように見えるんでしたっけ。
だとしたらこの値はNoneの受入が可能(Noneに意味がある)ので、SkipJsonSchema外さないとかもです。

もしそうっぽかったら別PRで・・・!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SkipJsonSchema[None]を付けるとJsonのnullが受け入れ不可能に見えるという感じです。
また、Fieldにデフォルト値が設定されている場合はオプショナルになります。

なのでスキーマー上はキーが存在しないか数値であるが正しい挙動になります。
(実際の挙動はnullを受け入れnullを出す)

このままでも問題ないような気がしますがモデルにSkipJsonSchema[None]を付けて回ったのは今までのスキーマーを破壊しないためなので0.20の新機能であるキーにSkipJsonSchema[None]は付ける必要は無さそうですね…

Copy link
Member

@Hiroshiba Hiroshiba Jun 24, 2024

Choose a reason for hiding this comment

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

なるほどです!!

たぶんpauseLengthSkipJsonSchema[None]は外した方が良さそうですね!!
プルリク出そうと思います!

pauseLengthScaleの方にSkipJsonSchema[None]をつける手もありそうだけど、こっちはつけない方がいいかなぁ・・・。
難しいですね。。

Copy link
Contributor

Choose a reason for hiding this comment

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

SkipJsonSchema[None]を付けるとJsonのnullが受け入れ不可能に見える ...
...
(実際の挙動はnullを受け入れ ...

の検証コードを書きました。
結論として @sabonerune さんの仰る通りで間違いないです👍️

import json

from fastapi import FastAPI
from fastapi.testclient import TestClient
from pydantic import BaseModel, Field
from pydantic.json_schema import SkipJsonSchema

app = FastAPI()


class Container(BaseModel):
    attr_required: int
    attr_optional: int | SkipJsonSchema[None] = Field(default=None)


@app.post("/container")
def post_container(container: Container) -> None:
    print(f"recieved: {container}")
    return None


client = TestClient(app)

res_post_1 = client.post("/container", json={"attr_required": 11, "attr_optional": 12}).json()
# recieved: attr_required=11 attr_optional=12
print(res_post_1)
# None

res_post_2 = client.post("/container", json={"attr_required": 11}).json()
# recieved: attr_required=11 attr_optional=None
print(res_post_2)
# None

res_post_3 = client.post("/container", json={"attr_required": 11, "attr_optional": None}).json()
# recieved: attr_required=11 attr_optional=None
print(res_post_3)
# None

@Hiroshiba Hiroshiba merged commit 6208379 into VOICEVOX:master Jun 24, 2024
4 checks passed
@sabonerune sabonerune deleted the fix/audio-query branch June 24, 2024 12:24
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Nov 19, 2024
VOICEVOX/voicevox_engine#1308VOICEVOX/voicevox_engine#1425 の一部
を参考にコードを書いた。

COREへのENGINE機能移植の一環として、 @rokujyushi さんと @sabonerune
さんの許諾のもと、MITライセンスとしてライセンスする。

Co-Authored-By: 黒猫大福 <[email protected]>
Co-Authored-By: sabonerune <[email protected]>
Refs: VOICEVOX#874 (comment)
Refs: VOICEVOX#874 (comment)
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Nov 20, 2024
VOICEVOX/voicevox_engine#1308VOICEVOX/voicevox_engine#1425 の一部
を参考にコードを書いた。

TODO: @X-20A さんの許諾を取るか、"hihoライセンス"経由で取り込む旨を書く

Co-Authored-By: VOICEVOX#874 (comment)
Co-Authored-By: sabonerune <[email protected]>
Refs: VOICEVOX#874 (comment)
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Nov 20, 2024
VOICEVOX/voicevox_engine#1308VOICEVOX/voicevox_engine#1425 の一部
を参考にコードを書いた。

TODO: @X-20A さんの許諾を取るか、"hihoライセンス"経由で取り込む旨を書く

Co-Authored-By: X-20A <[email protected]>
Co-Authored-By: sabonerune <[email protected]>
Refs: VOICEVOX#874 (comment)
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Nov 21, 2024
VOICEVOX/voicevox_engine#1308VOICEVOX/voicevox_engine#1425 の一部
を参考にコードを書いた。

@Hiroshiba さんと以下の2名の許諾のもと、 VOICEVOX#874 にのっとりMITライセンスと
してライセンスする。

* @X-20A (VOICEVOX/voicevox_engine#1308)
* @sabonerune (VOICEVOX/voicevox_engine#1425)

Co-Authored-By: X-20A <[email protected]>
Co-Authored-By: sabonerune <[email protected]>
Co-authored-by: Hiroshiba <[email protected]>
Refs: VOICEVOX#874 (comment)
Refs: VOICEVOX#874 (comment)
qryxip added a commit to VOICEVOX/voicevox_core that referenced this pull request Nov 23, 2024
feat: `pause_length{,_scale}`をデフォルト値限定で受け入れる

`AudioQuery`に`pause_length{,_scale}`を追加する。ただしそれぞれ`null`と
`1.`のみを許し、無音時間調整自体はまだ実装しない。

VOICEVOX/voicevox_engine#1308VOICEVOX/voicevox_engine#1425 の一部
を参考にコードを書いた。

@Hiroshiba さんと以下の2名の許諾のもと、 #874 にのっとりMITライセンスと
してライセンスする。

* @X-20A (VOICEVOX/voicevox_engine#1308)
* @sabonerune (VOICEVOX/voicevox_engine#1425)

Co-authored-by: X-20A <[email protected]>
Co-authored-by: sabonerune <[email protected]>
Co-authored-by: Hiroshiba <[email protected]>
Refs: VOICEVOX/voicevox_engine#1308, VOICEVOX/voicevox_engine#1425
Refs: #874 (comment)
Refs: #874 (comment)
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.

AudioQueryのpauseLengthScaleにデフォルト値を設定する
3 participants