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

PythonのブロッキングAPIを実装 #706

Merged
merged 13 commits into from
Dec 9, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 5, 2023

内容

#702 で実装したブロッキングAPIをPython APIで使い、PyO3で提供するclassを次の二つに分けて提供するようにします。

-voicevox_core.blocking.{Synthesizer,…}
-voicevox_core.asyncio.{Synthesizer,…}

関連 Issue

#662
#702

その他

@@ -49,6 +49,9 @@ jobs:
run: |
cargo build -p voicevox_core_c_api -vv
maturin develop --manifest-path ./crates/voicevox_core_python_api/Cargo.toml --locked
# https://github.com/readthedocs/sphinx-autoapi/issues/405
Copy link
Member Author

@qryxip qryxip Dec 5, 2023

Choose a reason for hiding this comment

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

PRが出ているが、以下のコメントから約2ヶ月放置されている状態になっている。
readthedocs/sphinx-autoapi#406 (comment)

Copy link
Member Author

@qryxip qryxip Dec 8, 2023

Choose a reason for hiding this comment

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

あと、このworkaroundを実行するとコードとしては壊れて実行できなくなる(_rust.abi3.*よりも_rust/__init__.pyの方が優先されるらしい)。

Sphinxを騙すためだけにGHA上で実行。

Copy link
Member Author

@qryxip qryxip Dec 9, 2023

Choose a reason for hiding this comment

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

手元で生成するのに面倒が生じてきてるので、いっそのこと他言語ごと

cargo xtask doc [--only rust c python ...] [--open]

みたいな感じで生成できるようにした方がよいかもしれない。

@qryxip
Copy link
Member Author

qryxip commented Dec 5, 2023

ひとまず

  • voicevox_core.blocking.OpenJtalk
  • voicevox_core.blocking.UserDict

だけ実装しました。次のように使えるはずです。

from voicevox_core.blocking import OpenJtalk

open_jtalk = OpenJtalk("…")
import voicevox_core

open_jtalk = voicevox_core.blocking.OpenJtalk("…")

@qryxip
Copy link
Member Author

qryxip commented Dec 5, 2023

  • 4b748f5: voicevox_core.{OpenJtalk,Synthesizer,UserDict,VoiceModel}voicevox_core.asyncioに移動。

@qryxip
Copy link
Member Author

qryxip commented Dec 6, 2023

blockingasyncioは実装し終えました。

example/pythonとテストをどうしようか考えています。

exampleの方はブロッキング版に統一するというのもよいですが、run-blocking.pyとrun-asyncio.pyに分けるという手もあると思います。

テストの方ですが、ブロッキング版classをasyncに包むラッパーを作り、ABCを定義してasyncio版と同等に扱うといった感じで両方のAPIのテストをできないかと考えています。

@Hiroshiba
Copy link
Member

楽しみです!!!

example/pythonとテストをどうしようか考えています。
exampleの方はブロッキング版に統一するというのもよいですが、run-blocking.pyとrun-asyncio.pyに分けるという手もあると思います。

サンプルの目的的には、簡単な方のブロッキング版はマストで用意してあげたいなと感じました!
asyncio版の方はあった方が良いなと感じました! けどまあ時間の都合などで難しかったりしたらなしでもいいのかなと・・・!

テストの方ですが、ブロッキング版classをasyncに包むラッパーを作り、ABCを定義してasyncio版と同等に扱うといった感じで両方のAPIのテストをできないかと考えています。

良さそう!

@qryxip
Copy link
Member Author

qryxip commented Dec 7, 2023

exampleの方はrun.pyとrun-asyncio.pyに分けました。

テストの方は...ABCは難しいことがわかりました。型制約の記述がPythonでは無理。

もう一つ考えている方法として、こういうのを振り回すことを考えています。

# `blocking`と`asyncio`に同じ引数を与え続け、出力が同じかどうかをassertし続ける
@dataclasses.dataclass
class Pair(Generic[B, A]):
    blocking: B
    asyncio: A

@Hiroshiba
Copy link
Member

なるほどです!
ちょっとそちらの方法が具体的にどんな感じかイメージは湧いてないのですが、もしくは片方だけのテストにするというのもありなのかなと思いました!!

@qryxip qryxip marked this pull request as ready for review December 8, 2023 15:38
@qryxip
Copy link
Member Author

qryxip commented Dec 8, 2023

9c99edc

ブロッキング版のテストをコピペで用意しました。各ファイルには対(blocking <-> asyncio)になるファイルについてコメントするようにしています。

Python API内のRustコード自体が結構重んでいることを考えると、これが無難であると結論付けた方がよいのかなと思いました。

@@ -262,6 +262,7 @@ jobs:
runs-on: ${{ matrix.os }}
defaults:
run:
shell: bash
Copy link
Member Author

Choose a reason for hiding this comment

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

どうやら今まで、このjob全体がPowerShellで実行されていた模様...
https://github.com/VOICEVOX/voicevox_core/actions/runs/7143365778/job/19454675377?pr=706

image

@@ -9,26 +9,10 @@ if TYPE_CHECKING:
AudioQuery,
SpeakerMeta,
StyleId,
SupportedDevices,
UserDict,
Copy link
Member Author

@qryxip qryxip Dec 9, 2023

Choose a reason for hiding this comment

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

このUserDictについては元々このファイル自体(旧_rust.pyi, 現_rust/asyncio.pyi)で定義されているはずで、Pyrightがエラーを吐く状態だった。
(mypyは試してはいないが、多分同様に駄目な気がする)

修正として別PRに分けるかどうかだが、どの道このPRから循環どころか正しくもなくなる(voicevox_core.UserDictは存在しなくなる)ので、それへの適応ということでこのPRに含めても良いと思った。

example/python/run-asyncio.py Outdated Show resolved Hide resolved
@qryxip qryxip requested a review from Hiroshiba December 9, 2023 17:14
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!!!!!!!

お疲れ様でした!!!!すごく良いですね!!!!

テストコードがblocking/async版でコピーになっている点ですが、まあ他の部分のコードもコピペだし、コード行数的にはそんなに長くないので、一旦全然良いのかなと思いました!
でもまあ(テストだけでなくいろいろと)なんとかしてコード量を減らせないか、ちょっと考えても良いのかなとも思いました!
とりあえずお疲れ様でした!!!

@Hiroshiba Hiroshiba merged commit 00a1c53 into VOICEVOX:main Dec 9, 2023
32 checks passed
@qryxip qryxip mentioned this pull request Mar 17, 2024
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