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

追加: 速度ベンチマーク #1240

Merged
merged 18 commits into from
May 27, 2024
Merged

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 13, 2024

内容

概要: 速度ベンチマークの追加

#1129 にて速度低下が問題視され、いくつかの解決方法が提案されている。
しかしその前提となる速度ベンチマークは各自に任されており、統一的測定がなされていない。
CI と同様に、benchmarking も as a code として実現すべきである。

このような背景から、コードによる速度ベンチマークの追加を提案します。

実装意図

より現実に近いベンチマークのために、2種類のエンジン起動法を用意した。
一方は TestClient を用いる方法であり、ネットワークを経由しないベンチマークである。これにより API 本体のベンチマークが可能になる。
他方は localhost を用いる方法であり、ネットワークを経由するベンチマークである。他プロセスで run.py を立ち上げる必要があるが、これで本番環境でのベンチマークが可能になる。
この 2つは use_localhost 引数で制御している。

実装詳細意図

(resolved type: ignoreclient.get() で利用している。型推論がなぜか上手くいかないため(解決法募集中)。)

ベンチマーク結果例

scode ➜ /workspaces/voicevox_engine (master) $ python -m test.benchmark.speed.speaker
Warning: cpu_num_threads is set to 0. Setting it to half of the logical cores.
Info: Loading core 0.15.3.
`GET /speakers` fakeserve: 0.0237 sec
`GET /speakers` localhost: 0.0361 sec
Warning: cpu_num_threads is set to 0. Setting it to half of the logical cores.
Info: Loading core 0.15.3.
全話者 `GET /speaker_info` fakeserve: 0.569 sec
全話者 `GET /speaker_info` localhost: 1.118 sec
Warning: cpu_num_threads is set to 0. Setting it to half of the logical cores.
Info: Loading core 0.15.3.
全話者 `GET /` fakeserve: 0.076 sec
全話者 `GET /` localhost: 0.571 sec

関連 Issue

@tarepan tarepan requested a review from a team as a code owner May 13, 2024 10:56
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 13, 2024 10:56
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です!!
コードに関していくつかコメントしました!

あとtest/ディレクトリはテストファイルがある場所で、ベンチマークはテストではない気がちょっとしました。
かなり立派ですし、benchmarkディレクトリを作っちゃっても良いかも?
(本当にテストを書く予定とかでしたらこのままでも!)

ちょっと手元でも試してみます!

test/benchmark/speed/request.py Outdated Show resolved Hide resolved
test/benchmark/speed/utils.py Outdated Show resolved Hide resolved
test/benchmark/setup.py Outdated Show resolved Hide resolved
test/benchmark/setup.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented May 16, 2024

あとtest/ディレクトリはテストファイルがある場所で、ベンチマークはテストではない

本チェックは「手動の速度テスト(speed test)」と解釈しています。ゆえに test/ 下がベストと考えます。
理想的には製品版の非機能テストの一環として実行速度を自動監視できればと考えています。


@Hiroshiba
全指摘箇所の反映・ベンチマーク変化無しを確認しました。Re-review よろしくお願いします。

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 です!!

ちょっとコメントだけお願いできると!!

test/benchmark/engine_preparation.py Show resolved Hide resolved
test/benchmark/engine_preparation.py Outdated Show resolved Hide resolved
test/benchmark/engine_preparation.py Outdated Show resolved Hide resolved
test/benchmark/engine_preparation.py Outdated Show resolved Hide resolved
test/benchmark/engine_preparation.py Outdated Show resolved Hide resolved
test/benchmark/speed/request.py Outdated Show resolved Hide resolved
test/benchmark/speed/request.py Outdated Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented May 20, 2024

@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re2-review よろしくお願いします。

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 +55 to +57
warn_msg = "root_dirが未指定であるため、自動的に `VOICEVOX/vv-engine` を `root_dir` に設定します。"
warnings.warn(warn_msg, stacklevel=2)
root_dir = Path("VOICEVOX/vv-engine")
Copy link
Member

Choose a reason for hiding this comment

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

このフォールバックはほぼ100%の環境で意味をなさないので、無いほうが良いかもですね・・・。
root_dir無指定ならエラー、あるいはfakeを実行しない形が使いやすいかもとか思いました!

test/benchmark/speed/request.py Show resolved Hide resolved
# 実行コマンドは `python -m test.benchmark.speed.speaker` である。
# `server="localhost"` の場合、本ベンチマーク実行に先立ってエンジン起動が必要である。
# エンジン起動コマンドの一例として以下を示す。
# (別プロセスで)`python run.py --voicevox_dir=VOICEVOX/vv-engine`
Copy link
Member

Choose a reason for hiding this comment

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

こちらも

Comment on lines +14 to +16
def execute() -> None:
"""計測対象となる処理を実行する"""
client.get("/speakers", params={})
Copy link
Member

Choose a reason for hiding this comment

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

これ関数の方の名前を良い感じにすればコメントなくせるかもですね!
target_process()とか・・・?

@Hiroshiba Hiroshiba merged commit 93e3347 into VOICEVOX:master May 27, 2024
4 checks passed
@tarepan tarepan deleted the add/benchmark_speed branch May 27, 2024 08:38
@tarepan
Copy link
Contributor Author

tarepan commented May 27, 2024

@Hiroshiba
自動マージすり抜けのエラーが出たため、hotfix を出しました (#1338)。

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