refactor: data modelを分離 by qwerty2501 · Pull Request #256 · VOICEVOX/voicevox_engine

qwerty2501 · 2021-12-29T17:56:36Z

内容

APIで返す型と内部で使う型が同じだとフィールドをおいそれと変えることが難しいので、型を分離する

Pros

APIのデータ構造を変えることなく内部で型を好きに変えることができる

Cons

初見の人になんで似たような型があるのか疑問を持たれる
初見の人になんで詰替え処理してるのか疑問を持たれる

その他

full_context_label.pyにある型もmodel.pyに統合してしまいたいが、変更量次第？

qwerty2501 · 2021-12-29T18:19:28Z

とりあえず分離したけど、model.pyの方の型はBaseModelとかFieldとかなくして、dataclassにしたほうが良さそう

voicevox_engine/model.py

refs VOICEVOX#256

coveralls · 2021-12-30T13:07:59Z

Pull Request Test Coverage Report for Build 1642267046

102 of 109 (93.58%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.9%) to 87.5%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
voicevox_engine/webapi/fastapi_model.py	102	109	93.58%

Totals
Change from base Build 1633757091:	0.9%
Covered Lines:	784
Relevant Lines:	896

💛 - Coveralls

github-actions · 2021-12-30T13:09:29Z

Coverage Result

Resultを開く

Name	Stmts	Miss
voicevox_engine/init.py	0	0
voicevox_engine/acoustic_feature_extractor.py	85	0
voicevox_engine/dev/synthesis_engine/init.py	2	0
voicevox_engine/dev/synthesis_engine/mock.py	30	1
voicevox_engine/full_context_label.py	162	3
voicevox_engine/kana_parser.py	87	1
voicevox_engine/model.py	64	6
voicevox_engine/mora_list.py	4	0
voicevox_engine/preset/Preset.py	12	0
voicevox_engine/preset/PresetLoader.py	34	1
voicevox_engine/preset/init.py	3	0
voicevox_engine/synthesis_engine/init.py	5	0
voicevox_engine/synthesis_engine/forwarder.py	76	66
voicevox_engine/synthesis_engine/make_synthesis_engine.py	23	18
voicevox_engine/synthesis_engine/synthesis_engine.py	108	0
voicevox_engine/synthesis_engine/synthesis_engine_base.py	55	6
voicevox_engine/utility/init.py	2	0
voicevox_engine/utility/connect_base64_waves.py	35	3
voicevox_engine/webapi/init.py	0	0
voicevox_engine/webapi/fastapi_model.py	109	7
TOTAL	896	112

refs VOICEVOX#256

qwerty2501 · 2021-12-30T17:44:54Z

とりあえずデータ型を分離して、変換関数を実装しました
以下判断お願いします

この変更を取り込むべきか
model.pyの方の型はBaseModelとかFieldとかなくして、dataclassにするべきか
full_context_label.pyにある型もmodel.pyに統合するべきか(PR分けたほうが良さそう)

Hiroshiba · 2021-12-30T18:29:26Z

この変更を取り込むべきか

たしかに変更量は結構大きめですが、取り込んじゃいましょう！！

ただもうちょっと実装の負担を減らせると嬉しいかなと思いました。
たとえばmora_modelを変換するためにfrom_mora_modelやto_mora_modelのglobal関数を実装すると、使用者が対応するコンバータを探す手間が増えることに気づきました。
なので各fastapi modelクラスにto_engineとfrom_engineのようなメソッドを付けちゃっても良いかもと感じました。
（汎用convert関数を作って中で型を見て変換、ということもできるのですが、こうするとpythonの型推論が壊れそうなので。。）

model.pyの方の型はBaseModelとかFieldとかなくして、dataclassにするべきか

dataclassにしちゃったほうが良いと思います。
ただ、Modelのプロパティ値とdataclassのプロパティ値は微妙に型が合わなそうなので、一旦ModelのままにしてFIXMEコメントを残すと良いのかなと感じました。

full_context_label.pyにある型もmodel.pyに統合するべきか(PR分けたほうが良さそう)

full_context_labelはOpenJTalkに引っ張られる仕様なため、エンジン用とはまた変わってきそうな気もします。
なのでそのままの方が良いかもと思いました。

qwerty2501 · 2021-12-30T19:22:59Z

なので各fastapi modelクラスにto_engineとfrom_engineのようなメソッドを付けちゃっても良いかもと感じました。

~engineだと何の型にするのか想像しにくいのではないでしょうか？ model.py にあるのだからto_model,from_modelにするか、modelが紛らわしいのならmodel.pyの名前を例えばdomain.pyにし、to_domain,from_domainにしたほうが良さそうに感じました。
また、List系だとクラスに変換関数を生やすのはちょっと違和感があるんですがどうしましょうか。

ただ、Modelのプロパティ値とdataclassのプロパティ値は微妙に型が合わなそうなので、一旦ModelのままにしてFIXMEコメントを残すと良いのかなと感じました。

👍

full_context_labelはOpenJTalkに引っ張られる仕様なため、エンジン用とはまた変わってきそうな気もします。
なのでそのままの方が良いかもと思いました。

このPRではやりませんが、残すのであればそれらの型名にプレフィックスでOpenJTalkをつけたほうが良さそうですね。似たような型があるので混同しそうです

Hiroshiba · 2021-12-30T19:58:37Z

~engineだと何の型にするのか想像しにくいのではないでしょうか？

例えばMoraクラスにto_engine()メソッド（Mora.to_engine()）を作って、エンジン用のMora型に変えるというのを想像していました！

…o_engineとして実装した若干名前に違和感はあるものの、特段こだわりがあったわけではないのでとりあえず修正 List系はあってもなくても書く量が変わらないのとクラスメソッドとして生えていたら不自然だったので削除した

…o_engineとして実装した若干名前に違和感はあるものの、特段こだわりがあったわけではないのでとりあえず修正 cyclic importになったのでもとの実装は削除した List系はあってもなくても書く量が変わらないのとクラスメソッドとして生えていたら不自然だったので削除した

qwerty2501 · 2021-12-31T06:21:39Z

from_engine,to_engineメソッドを各クラスに実装しましたcyclic importになったのでもとの実装は消しました

…o_engineとして実装した cyclic importになったのでもとの実装は削除した List系はあってもなくても書く量が変わらないのとクラスメソッドとして生えていたら不自然だったので削除した

Hiroshiba

大丈夫そうです！
後世に伝えるにあたってちょっと変えれそうなところをコメントしてみました。

run.py

test/test_fastapi_model.py

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>

これでわからなければコメントを追加する必要あり

Hiroshiba

LGTM！！

aoirint

LGTM！！

This reverts commit ff00ad6.

refacter: data modelを分離

edd949e

qwerty2501 mentioned this pull request Dec 29, 2021

create_kanaを疑問文に対応してaudio_query時に？が母音に書き換わってしまうのを抑止するようにした #255

Closed

qwerty2501 commented Dec 29, 2021

View reviewed changes

voicevox_engine/model.py Show resolved Hide resolved

run.pyに使用する型をfastapi用のmodelに変更した

da3af19

refs VOICEVOX#256

型を分離している理由を記載した

9d9bb59

refs VOICEVOX#256

qwerty2501 marked this pull request as ready for review December 30, 2021 17:36

converterのテストケース追加した

7642e52

qwerty2501 force-pushed the refacter/data-model branch from ea8d8fa to 7642e52 Compare December 30, 2021 17:42

converter通すように修正

c50ef6e

各fastapi_modelのclassにto_modelとfrom_modelを生やした

c3f5ddc

qwerty2501 force-pushed the refacter/data-model branch from f022063 to f0ee782 Compare December 31, 2021 06:20

feat: コメントの加筆と修正を行った

8cf983d

qwerty2501 force-pushed the refacter/data-model branch 2 times, most recently from 17f9c31 to 8cf983d Compare December 31, 2021 08:37

fixme追加

359c793

qwerty2501 changed the title ~~refacter: data modelを分離~~ refactor: data modelを分離 Dec 31, 2021

Hiroshiba requested changes Dec 31, 2021

View reviewed changes

run.py Outdated Show resolved Hide resolved

test/test_fastapi_model.py Outdated Show resolved Hide resolved

test/test_fastapi_model.py Outdated Show resolved Hide resolved

qwerty2501 and others added 2 commits January 1, 2022 09:28

Fixed typo

2cecd49

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>

givenとの比較は今後内部型との差異が出た場合に問題になるため不要

df2845a

assert関数の名前と変数名が分かりづらかったので変更

a62ee68

これでわからなければコメントを追加する必要あり

Hiroshiba approved these changes Jan 1, 2022

View reviewed changes

Hiroshiba requested review from aoirint and y-chan January 1, 2022 17:18

aoirint approved these changes Jan 1, 2022

View reviewed changes

Hiroshiba merged commit ff00ad6 into VOICEVOX:master Jan 2, 2022

qwerty2501 deleted the refacter/data-model branch January 2, 2022 06:59

takana-v mentioned this pull request Jan 3, 2022

0.10.preview.8のエンジンが起動しない #264

Closed

3 tasks

qwerty2501 restored the refacter/data-model branch January 4, 2022 01:18

takana-v added a commit to takana-v/voicevox_engine that referenced this pull request Jan 4, 2022

Revert "refactor: data modelを分離 (VOICEVOX#256)"

b7a78c9

This reverts commit ff00ad6.

takana-v mentioned this pull request Jan 4, 2022

Revert "refactor: data modelを分離 (#256)" #266

Merged

Hiroshiba pushed a commit that referenced this pull request Jan 4, 2022

Revert "refactor: data modelを分離 (#256)" (#266)

16b1d5a

This reverts commit ff00ad6.

takana-v mentioned this pull request Jan 23, 2022

coreのctypes実装を追加＆古いコアを利用できるようにする #254

Merged

takana-v mentioned this pull request May 5, 2022

Nuitkaのバージョンを上げる #393

Closed

3 tasks

Conversation

qwerty2501 commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

内容

Pros

Cons

その他

Uh oh!

qwerty2501 commented Dec 29, 2021

Uh oh!

Uh oh!

coveralls commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1642267046

💛 - Coveralls

Uh oh!

github-actions bot commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Result

Uh oh!

qwerty2501 commented Dec 30, 2021

Uh oh!

Hiroshiba commented Dec 30, 2021

Uh oh!

qwerty2501 commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hiroshiba commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qwerty2501 commented Dec 31, 2021

Uh oh!

Hiroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hiroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

aoirint left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qwerty2501 commented Dec 29, 2021 •

edited

Loading

coveralls commented Dec 30, 2021 •

edited

Loading

github-actions bot commented Dec 30, 2021 •

edited

Loading

qwerty2501 commented Dec 30, 2021 •

edited

Loading

Hiroshiba commented Dec 30, 2021 •

edited

Loading