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

refactor: API と音声ライブラリを分離 #1249

Open
tarepan opened this issue May 16, 2024 · 6 comments
Open

refactor: API と音声ライブラリを分離 #1249

tarepan opened this issue May 16, 2024 · 6 comments
Labels

Comments

@tarepan
Copy link
Contributor

tarepan commented May 16, 2024

内容

概要: API と音声ライブラリを分離してリファクタリングしたい

現在の VOICEVOX ENGINE は VVAPI と音声ライブラリ(コア)を pydantic 経由で密結合させがちである。
その結果、内部のリファクタリングが不可能になったり、コアと独立した変更を ENGINE に加えられなくなったりしている(例: #1121 (comment) )。

これは BaseModel サブクラスを API 定義と内部型に二重利用していることが本質的な原因であり、このことは長く指摘されてきた(#262)。
二重利用はクラス詰め替え等を省略できる短期的効果があったが、長期的デメリットが顕在化してきている。上記問題点はその一例であり、他にも pydantic のバージョン移行に伴う負債が発生している。クラス詰め替えはごくありふれた数行の処理であり、面倒なだけでメンテコストも実際のところはごく小さい。すなわち、二重利用はメリット < デメリットとなりつつある。

このような背景から、API と音声ライブラリの分離によるリファクタリングを提案します。

Pros 良くなる点

  • VVAPI とエンジン実装の分離による自由度増
  • Model 影響範囲減によるメンテ性向上

Cons 悪くなる点

  • クラス詰め替えコード増

実現方法

  • API ごとに音声ライブラリと分離

VOICEVOXのバージョン

0.19.0

その他

Go/NoGo 判断の一助として supported_devices を分離する draft PR を作成しました(#1250)。必要であればご活用ください。

@Hiroshiba
Copy link
Member

実装方針について迷っています!
データ構造を詰め替えるパターンについていろいろ考察されてたりメリット・デメリット・実際の落とし所とかが書かれてるブログ記事とかありそうでしょうか 🙇

というのも自分が設計の勉強をしたことがなく・・・。
レイヤーが変わるときにデータ構造等を変えるメリットはわかるし、たとえばdeviceの例だと普通にリファクタリングとして良さそうに感じるのですが、
例えば全データで2種用意すべきなのかとか、必要になってから実装でも良いのか、あるいは共通のBase構造体を持っといて差が出たときだけ差分を変えるみたいな折衷案があったりするのかなどがわからず。。

メンテがしんどくならず、しかし機能実装は容易なラインを見極めたく、資料があれば・・・という感じです 🙇

@tarepan
Copy link
Contributor Author

tarepan commented May 21, 2024

例えば全データで2種用意すべきなのかとか、必要になってから実装でも良いのか、あるいは共通のBase構造体を持っといて差が出たときだけ差分を変えるみたいな折衷案があったりするのかなど

妥当な疑問点だと感じます。

データ構造を詰め替えるパターンについていろいろ考察されてたりメリット・デメリット・実際の落とし所とかが書かれてるブログ記事とかありそうでしょうか

本問題そのものは「FastAPI が API 型を Python クラスで表現させる(= API 定義クラスを内部データ構造に流用する魅力がある)」という特殊条件に由来しているため、比較的マイナーな問題です。なので私が紹介できる記事等は無さそうです。
一方で、この問題を抽象化すると「インターフェイスをどこでどう切るか」というメジャーな問題になります。この手の話題だと基本方針は Uncle Bob の The Clean Architecture が有名どころの記事かと思います。具体的事例を挙げつつ詰め替え可否を検討しているブログは私は寡聞にして知らないです。

@Hiroshiba
Copy link
Member

Hiroshiba commented May 22, 2024

ありがとうございます!

レイヤーが分かれているとは感じます。
詰め替えの重要度がわからない感じでいます。

少なくとも、詰め替え前と詰替え後でデータ構造が異なる場合は、構造体を2個定義してでも詰め替えるべきだと感じました!
全く同じ場合は2つ定義すべきか判断がついていないです。

@tarepan
Copy link
Contributor Author

tarepan commented May 22, 2024

詰め替え前と詰替え後でデータ構造が異なる場合は、構造体を2個定義してでも詰め替えるべき

👍️
同意です。
CORE 出力 JSON dict へ属性アクセスして API 用構造体の init() 引数へ渡すタイプのコードはこれに該当するため、リファクタリングしたいと思います。

全く同じ場合は2つ定義すべきか判断がついていない

ブログ等が今のところ挙がっておらず判断根拠が足りないので、「同一である間はひとまず使い回してもよい」「同一じゃなくなるタイミングが来たら構造体 2 つ化を強く推奨」をレビュアーの暫定理解として持つのはどうでしょうか?
これでしばらく回してみて「2つ定義が綺麗に見えてくる」「1つ定義でそんなに問題起きない」といった知見を貯めるという意図です。

@tarepan tarepan added 状態:設計 設計をおこなっている状態 and removed 状態:必要性議論 必要性を議論している状態 labels May 25, 2024
@Hiroshiba
Copy link
Member

ブログ等が今のところ挙がっておらず判断根拠が足りないので、「同一である間はひとまず使い回してもよい」「同一じゃなくなるタイミングが来たら構造体 2 つ化を強く推奨」をレビュアーの暫定理解として持つのはどうでしょうか?
これでしばらく回してみて「2つ定義が綺麗に見えてくる」「1つ定義でそんなに問題起きない」といった知見を貯めるという意図です。

良さそうに感じました!

こんな感じのふわっとした取り決めを、どこかのドキュメントにふわっとまとめていけると嬉しいかもですねぇ。
./docs/設計メモ.mdとか作って、とか・・・?

@tarepan tarepan added 状態:実装者募集 実装者を募集している状態 and removed 要議論 実行する前に議論が必要そうなもの 状態:設計 設計をおこなっている状態 labels May 27, 2024
Copy link

本 Issue は直近 180 日間で活動がありません。今後の方針について VOICEVOX チームによる再検討がおこなわれる予定です。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants