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: コアを各機能モジュールに内蔵 #1391

Closed
tarepan opened this issue Jun 3, 2024 · 17 comments · Fixed by #1447
Closed

refactor: コアを各機能モジュールに内蔵 #1391

tarepan opened this issue Jun 3, 2024 · 17 comments · Fixed by #1447
Assignees

Comments

@tarepan
Copy link
Contributor

tarepan commented Jun 3, 2024

内容

TTSEngine を解体してリファクタリング

多数のリファクタリングの積み上げにより、現在の VOICEVOX ENGINE は機能モジュール間が疎結合になっている。特にコアのインターフェースは洗練され、また利用パターンが明確になってきている。典型的には以下のパターンで利用される:

  • 全コアを抱える core_manager を router へ注入
  • router 内 path operation 冒頭で core_version に基づき特定バージョンのコアを取得
  • コアのメソッドを叩いて得た値を用いて各機能実装をコール or コアそのものを引き渡し

TTSEngine は初期にリファクタリングされ、コアを TTSEngine.__init__() で受け取る設計になっている。
これは当時妥当な設計であったが、コアが洗練された現在では過剰に state をもつ設計になっている。
TTSEngine のインスタンス変数は self._core のみであるため、コア受け入れを廃止すれば stateless に関数で音声合成ができる。これは音声合成機能の実装を大幅に簡略化する。

このような背景から、 TTSEngine を解体するリファクタリングを提案します。

Pros 良くなる点

  • 音声合成の見通し大幅改善

Cons 悪くなる点

  • 無し

実現方法

  • コアをメソッド引数へ変更
  • コアの init 引数を廃止
  • TTSEngine の解体による廃止

VOICEVOXのバージョン

0.19.0

その他

以下の先行 PR に依存しています:

@tarepan tarepan added 機能向上 要議論 実行する前に議論が必要そうなもの 状態:必要性議論 必要性を議論している状態 labels Jun 3, 2024
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 15, 2024

たしかにコアのラッパーとしてのTTSEngineを作るのではなく、コアを引数にした関数群にする手はあると思いますが、これだけだと短所が目立つ気がします。
TTSEngineにすれば、まとまりを作れて、音声合成系のrouterからコアの知識を剥がせるので。(一部剥がしきれて無さそうですが・・・)
もちろんTTSEngineを定義しなくて良いメリットはありますが、トントンかラッパーのが便利なのかなぁと。

たぶんどこか僕が認知してない部分に辛さがあり、それが解消できるのかなと想像してます。そこを知りたいです。
あるいはもうちょっと先の設計が思い浮かんでいたらぜひ知りたいです!

あと関係ないですが、TTSEngineと呼ぶのをやめてSynthesizerとかにすれば、「エンジン」という名前が衝突する問題は解決するかもです。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 16, 2024

TTSEngine 廃止後の router がどうなるかイメージを共有できていない気がするので、before/after を提示します:

before

    def synthesis(..., core_version) -> FileResponse:
        engine = tts_engines.get_engine(core_version)
        wave = engine.synthesize_wave(
            query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
        )

after

    def synthesis(..., core_version) -> FileResponse:
        core = core_manager.get_core(core_version)
        wave = synthesize_wave(
            core , query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
        )

音声合成系のrouterからコアの知識を剥がせる

上記のように、音声合成系のrouterからコアの知識を剥がせる具合は一切変化しません。(追記: 「コア」と「エンジン」を空目しました)

#1317 により「core_version=None からバージョン文字列への変換」は TTSEngineManager 内ではなく router 内で明示的におこなう方針となりました(大元: #1234 (comment) by @Hiroshiba )。core_version の変換は core_manager の責務であるため、これにより 音声合成系 router の全 API が core_manager を利用するようになります。
ゆえに「音声合成系のrouterからコアの知識を剥がせる」を利点とするのは今後の方向性と逆を志向しているようにみえます。

TTSEngineにすれば、まとまりを作れて

音声合成は tts_pipeline.tts_engine モジュールへ既に集約されています。


私視点だと「(今となっては)無意味に TTSEngine でラップする形であり、見通しを悪くしているだけ」に見えています。
特段ツラミがあるというより、消すデメリットが無いので冗長なラッパーを削除する、という感じです。

@Hiroshiba さんの指摘する「短所が目立つ」がピンと来ていないので、詳しく指摘して頂けると幸いです🙇

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 18, 2024

一番伝えたかった点はここです・・・!

もちろんTTSEngineを定義しなくて良いメリットはありますが、トントンかラッパーのが便利なのかなぁと。

まあつまり、routerとのやりとりでエンジンインスタンスを仲介させるか、させないか、どっちが良いのかですよね。
いちばんの論点はここ↓かなぁ。

音声合成系のrouterからコアの知識を剥がせる

最近のrouterがコアを見てるのはそういう方針というより、そういう実装に寄ってるだけな認識です。
コアを剥がすことも、TTSEngineインスタンスをなくすことも、どっちにも倒せるはず。

他に差は・・・TTSEngineにステートを持たせたくなるかどうかでしょうか・・・。


うーーーーーーーーーーーーーーーんどっちだろう!!!!!
なんとなく今のTTSEngineに該当するのを一層噛ませておいたほうが柔軟な設計ができるだろうなとは感じます。
すみません、ちょっといろいろ思いつくことを書いてみます!

パッと思いつくのはバルク処理とかでしょうか。
WebAPIに推論リクエストが同時に届いたとき、個別に推論するよりも蓄積させて1回推論したほうが早い。
そういうのを実装する層がほしいときに、おそらくTTSEngineの層の仕事になる・・・?
けど・・・・・少なくとも1〜2年は実装しなそう・・・・・・・。なのでこれはまあ考えなくても良さそう・・・?

他にはコアが推論キャンセル可能になったときとかどこかにステートが必要そう。
エンジン部分がステートを持つのが良いかはわからないけど。
これは早ければ来年とかには実装可能になるかも。

あとframe_synthesisは原理上、逐次的に生成できるんですよね。
コアが、というよりonnxモデルが逐次生成に対応してないだけで、原理上できる。
これはソングの遅延を少なくするために実装したい気持ちがあります。早ければ年始か年末くらいに実装・・・できるかも・・・?
そのときstateが必要になりそうで、それはエンジンが担うのが良さそう・・・?どうだろう。。。

とはいえ現状解体は可能なのもそうで、2年間変わってない・・・


まとめると、「冗長性をなくす」わけで、パッと判断できない感じな気がしました!

もし設計が得意でしたら、上でいくつかあげた例を実装する場合、TTSEngineが良いか、他の何かが良いかとかお聞きしたいです。。
あるいは、必要になったときにすぐ層を足せそうか、とか。。。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 18, 2024

あ、あとTTSEngineが無いことによりできるようになることがあれば知りたいです 🙏
TTSEngineManagerをなくせる、とか。

あっ あとCancellableEngineは維持できるかも気にかけると良いかも・・・?

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

コアの知識を剥がせる

コアを抱えるか渡すかでコード上は以下の違いが生まれます。

        wave = engine.synthesize_wave(
            query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
        )
        wave = synthesize_wave(
            core, query, style_id, enable_interrogative_upspeak=enable_interrogative_upspeak
        )

違いは core 引数のみであり、剥がされる知識は「コアを渡す」かと思います。
「コアを渡す」という知識を持たなくて良いことが大きなメリットである、という理解であっているでしょうか?


今のTTSEngineに該当するのを一層噛ませておいたほうが柔軟な設計ができる

👍️
YAGNI と将来拡張性の天秤だと思うので、1 方針としてあり得ると思います。

バルク処理

👍️
バルク処理では ENGINE のシングルトンに buffer を持たせる設計になるため、ENGINE のシングルトンである TTSEngine が実装箇所になりそうです。

コアが推論キャンセル可能

CancellableEngineTTSEngine を抱える(≠ 継承)構造になっているため、キャンセルに関する状態は CancellableEngine に集約するのが良さそうに感じます(CancellableEngine 正直よくわかってない、TTSEngine 廃止でも維持は問題なくできる)。

逐次的に生成

逐次生成の場合、FastAPI の StreamingResponse を返す設計になるかと思います(ref)。
TTSEngine は ENGINE のシングルトンであるため、各レスポンスごとに Iterator インスタンスを要する StreamingResponse とはあまり関係無さそうに思います(TTSEngine に状態を持たせる設計にならない)。


TTSEngineが無いことによりできるようになること

以下の簡略化が可能になります:

  • TTSEngineManager の廃止
  • make_tts_engines_from_cores() の廃止
    • run.py 等の app 立ち上げ簡略化
  • generate_app(tts_engines) 引数の廃止

以下の機能追加が容易になります:

  • 音声ライブラリの動的な追加
    • 現在: 動的登録時に core_managertts_engine_manager の 2 つのマネージャが矛盾しない(例: 片側だけ登録失敗したケースのケア)ための仕組みを実装
    • 廃止後: core_manager.register_core() 叩くだけ

必要になったときにすぐ層を足せそうか

現在の TTSEngine はほぼ class method (状態はコアだけ)なので、再実装は面倒だけど難易度は低い感じになりそうです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 19, 2024

@sabonerune さんがDiscordでコメントしてくださった点を引用

元々はリポジトリをクローンしてエンジンクラスを差し替えればある程度簡単に別の合成音声エンジンをVOICEVOX APIに対応できる感じだったような気がする。(といってもそれだけでは不十分だったが)
今はEngineBaseがなくなって単なるCoreWapperの薄いラッパーになっている感じがあるからいらなくなった?(というよりいらなくなるように変えていった?)

たしかに、昔は変更が必要なとこだけ別クラスになっていたので、forkするときは大変かもです。
(そうするために作ってたかどうかは覚えてないです。。。。)

TTSEngineを解体する場合、それぞれの関数にコアが必須になり、コアが必要ないエンジンにとってはより不便になると思います。
routerとコアも密結合するので、全体的にVOICEVOX用に特化することになりそうです。
そもそもその辺りのポリシーを定めてなかったので、これから決めていくことになりそう。


あと、昔の設計と今の設計を見比べると、そもそも方針が大きく違ってそうでした。
0.14時代のコード

coresやmanagerがなく、enginesのみで管理されてました。
https://github.com/VOICEVOX/voicevox_engine/blob/release-0.14/run.py#L203

コアが不要なコードはSynthesisEngineBaseに、必要な処理はSynthesisEngineに書かれる傾向にありました。
split_moraとかはコアに無関係なのでBase側でも良さそうですが・・・。)

サンプリングレートがコアから毎回取得する形ではなく、全体で1つだけ持っておく方針でした。
https://github.com/VOICEVOX/voicevox_engine/blob/release-0.14/run.py#L123

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 19, 2024

@tarepan お待たせしました!

「コアを渡す」という知識を持たなくて良いことが大きなメリットである、という理解であっているでしょうか?

層を挟むことでコアと疎結合を保てる、という意図です。


設計について助かります!

逐次生成の場合、FastAPI の StreamingResponse を返す設計になるかと思います(ref)。
TTSEngine は ENGINE のシングルトンであるため、各レスポンスごとに Iterator インスタンスを要する StreamingResponse とはあまり関係無さそうに思います(TTSEngine に状態を持たせる設計にならない)。

レスポンス返す側は仰るとおりrouterに持たせる形だと思います。
ただお伝えしてなかったのですが、逐次生成するためには何かしらの計算結果を用いてコアを何度も叩く形になるので、stream response以外でもステートが必要になると思います。
(そういうことじゃないかも・・・?)

でもよく考えると、逐次生成はTTSEngineではなく、リクエストごとに何かしらのインスタンスを作る戦略がシンプルで良い気がしてきました!
キャンセルも、リクエストごとに何か作る設計のが良いかも。
バルク処理は外に持たせないとだけど、TTSEngineじゃなくても良いかも。

以下の簡略化が可能になります:
以下の機能追加が容易になります:

こちらもありがとうございます。コアとエンジンの一貫性をなくせるのは大きそうです。

ただ、先ほどのコメントを書くときに思い出したのですが、実は一連のリファクタリングがされる前はCoreManagerがなかったんですよね。
以前の/audio_query https://github.com/VOICEVOX/voicevox_engine/blob/release-0.14/run.py#L210-L233
今の/audio_query

@router.post(
"/audio_query",
response_model=AudioQuery,
tags=["クエリ作成"],
summary="音声合成用のクエリを作成する",
)
def audio_query(
text: str,
style_id: Annotated[StyleId, Query(alias="speaker")],
core_version: str | None = None,
) -> AudioQuery:
"""
音声合成用のクエリの初期値を得ます。ここで得られたクエリはそのまま音声合成に利用できます。各値の意味は`Schemas`を参照してください。
"""
engine = get_engine(core_version)
core = get_core(core_version)
accent_phrases = engine.create_accent_phrases(text, style_id)
return AudioQuery(
accent_phrases=accent_phrases,
speedScale=1,
pitchScale=0,
intonationScale=1,
volumeScale=1,
prePhonemeLength=0.1,
postPhonemeLength=0.1,
outputSamplingRate=core.default_sampling_rate,
outputStereo=False,
kana=create_kana(accent_phrases),
)

ぐるっと回ってきて違うとこに着地しようとしてる、って感じだと思います。

やる・やらないは置いといて、今のコードを読んだ感じも、EngineManagerのほうじゃなくCoreManagerの方を解体することもできる気がしました。


長々コメントしてしまってすみません。。。。。
もし提案issueを進める場合、エンジン中心からコア中心に変えるという判断になるのですが、それでよいのかまだ迷っています。

ちょっと一旦のコメントになってしまうのですが、エンジン側じゃなくCoreManager側を解体する視点はどうか聞けると嬉しいです!!🙇
もしそれでもリファクタリングが十分そうなら、APIとコアの密結合を防ぎつつリファクタリングができるので・・・・・・。。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

CoreManager側を解体する視点はどうか

可能かつ有効であると考えます。
二重管理が不要になり、「コア」という存在を実装詳細に押し込めるのでコードが綺麗になると思います。

ただし、この方針を取るなら ENGINE 全体でも同様の方針を取るべきと考えます。
例えば MetasStoreTTSEngine と同様の立ち位置にあるので、MetasStore もコアを内蔵する形になります。

version = core_version or core_manager.latest_version()
core = core_manager.get_core(version)
speakers = metas_store.load_combined_metas(core.speakers)

でないと コアが必要ないエンジンにとってはより不便になる といった、 CoreManager 廃止の意義がよくわからなくなるからです。「音声合成周りだけコアを内蔵し、他機能ではコアを分離する」はコントリビュータが混乱する原因になりそうです。

ゆえに CoreManager 解体は可能かつ有効であり、採用するならレポジトリ全体で適用されるべきと考えます。


(余談)
以前複数の PR で各機能マネージャーへの core_manager 内蔵や core 内蔵を提案したのですが、それらはどれも「コアは分離して必要な情報のみをメソッド引数で渡すと疎結合で良い」との方針で reject でした。この方針に従ってコア分離を進め、それが TTS まで行き着いてこの PR が提出された、という流れです。
なので私としては「内部実装の内部実装」であるコアが隠蔽されるという方針は好みです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 19, 2024

ただし、この方針を取るなら ENGINE 全体でも同様の方針を取るべきと考えます。
例えば MetasStore は TTSEngine と同様の立ち位置にあるので、MetasStore もコアを内蔵する形になります。

なるほどです!! ほんとですね!!
となるとMetasStoreもcore_versionを受け取る部分を作る感じになりそうですかね。。

うーーーーーーーーーーん・・・そうかぁ・・・。
これ系結構多いですよね、supported_deviceとか、initialize_speakerとか・・・。

あれ、MetasStoreにも内蔵する形はよくない気がしてきました。
それら全部にcore_versionを受け取るManager関数作ることになって大変になる気がする・・・?

以前(0.14時代)は全部SynthesisEngineに詰め込んでそうでした。
それでコアは隠蔽しきれて、かつManagerの乱立も防げたけど、アダプター用のメソッドを作りまくらないといけなそう?

たぶんManagerの乱立よりはEngineに全部押し付けるほうが楽そうな気はしますが・・・それで良いんだろうかという気持ちもありますね・・・。

(まとまってなくてすみません。。 🙇 )

@tarepan
Copy link
Contributor Author

tarepan commented Jun 19, 2024

Manager関数作る

はい、CoreManager 廃止方針を取った場合は metas_store_manager.get_store(core_version) で指定バージョンの MetasStore を取得する形になります。tts_engine_manager / TTSEngine と同じ形です。
コア内蔵方式を取るのであればこうならざるを得ないと考えます。

Engineに全部押し付けるほうが楽そう

この方針に反対です。
SynthesisEngine 時代は「音声合成機能とコアアクセス機能を 1 つのクラスに同居させ、このクラスを全体で引き回す」設計でした。
現在の ENGINE でこれを採用した場合、SynthesisEngine が全 router へ渡され、各 API が SynthesisEngine メソッドを叩きまくる形になります。すなわち SynthesisEngine は神クラスです。
機能的凝集の観点(音声合成とコアアクセスの同居)から、アーキテクチャとして正当化できないと考えます。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 20, 2024

なるほどです。

複数のManagerが出来上がると、TTSEngineManagerだけでも辛かったのがもっと更にいろんなManagerが出てくることになりそうで、それで良かったんだっけという感じになるかもです。
いろんなManagerが生まれて、SynthesisEngineに全責務を押し付けるかだと思ってます。
これのどちらがマシかをもう少し見積もりたいです。

仮にManagerを作る場合、Managerはいくつできそうでしょうか?
たぶんCoreManagerは続投し、TTSEngineManagerも続投なので、あとMetasStoreManager。
更にGET /supported_deviceとか、POST /initialize_speaker用のmanagerも必要・・・?

@tarepan
Copy link
Contributor Author

tarepan commented Jun 21, 2024

仮にManagerを作る場合、Managerはいくつできそう

次の仮定を起きます:

  • CoreManager は続投
  • コア出力値をほぼ API 出力とする場合は core_manager.get_core(x).the_value という形で利用(= ラップしない)

この仮定では、各クラスがコアを内蔵した結果として以下のマネージャークラスを要します:

  • CoreManager
  • TTSEngineManager
  • MetasStoreManager

よってマネージャーが 3 つになります。
GET /supported_device はコア出力の直接利用、POST /initialize_speakerTTSEngine 内での実装、となるためマネージャーを増やさなそうです)

@Hiroshiba
Copy link
Member

ありがとうございます!!
あれ、CoreManagerも復活するんですね。

現状整理してみます。

課題はコアをなるべく隠蔽したいことと、シンプルな実装にしたいことの二律背反。
コアを隠蔽したい理由は、コアがなくてもWebAPIを作りやすくするため。

以前の方法は全部をSynthesisEngineで隠蔽する手段。
完全に隠蔽できる。
けど話者情報・音声合成・音声合成の準備やその他情報が1クラスに集まる。

2つめの方法、コアを前提にしてSynthesisEngineをなくす手段。
最もシンプルになる。
けどコアとWebAPI実装が密結合になる。

3つめの方法、コアに依存する3クラス用のManagerをそれぞれ作る手段。
疎結合かつ一部は隠蔽できる。
けどManagerが増えてややこしくはなるのと、コアの隠蔽が完全ではなくなる。


CoreManager・EngineManager・MetasStoreManagerを作る方法ですが、コアをrouterに露出させるのは元の目的から考えると避けたほうが良さそうに思えてきました。
CoreManagerはなくして、EngineManagerとMetasStoreManagerだけにできるとより良いかも・・・?
(たぶんinitialize_speakerとかをCoreからEngineにアダプターする感じになる)
以前はEngineに話者情報が含まれてたのを、話者情報だけは別管理する形になりそう。

うーーーーーーーーん。。。。。。
あとそれはそれとして、コアとWebAPIを密結合させてシンプルにするのも魅力ではあるんですよね。。。。
決めが難しい。。。。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 22, 2024

CoreManagerはなくして、EngineManagerとMetasStoreManagerだけにできるとより良い

こちらの方針に基づいてインターフェースレベルの仮実装をしました。
その際、MetasStoreManager は不要であることがわかりました(TTSEngineManager と違って MockMetasStore の受け入れ不要のため)。このため露出するマネージャークラスは TTSEngineManager のみに限定できそうです。
また、以下の 2 点が課題として出てきました。

core_manager.versions()

課題: GET /core_versions API 用の core_manager.versions() を何で代替するか

tts_engine_manager.versions() を新規実装する? その場合 meta_store.versions() ではない理由は?

core.supported_devices

課題: GET /supported_devices API 用の core.supported_devices を何で代替するか

デバイス情報は合成器情報と解釈して TTSEngine.supported_devices() メソッドを生やす?

@Hiroshiba
Copy link
Member

仮実装・・・!!!

その際、MetasStoreManager は不要であることがわかりました

!!!! 嬉しみ!!!

core_manager.versions()
課題: GET /core_versions API 用の core_manager.versions() を何で代替するか

これは・・・難しいですね・・・
もしMetasStoreManagerが不要であれば、実装するならtts_engine_manager.versions()一択・・・?
そのままcore_versions変数を作って、それをrouter側に渡すという手もあるかも・・・?

core.supported_devices
課題: GET /supported_devices API 用の core.supported_devices を何で代替するか

こちらはTTSEngineに実装できると嬉しいかもです 🙇 🙇

@tarepan
Copy link
Contributor Author

tarepan commented Jun 23, 2024

嬉しみ!!!

👍️
最終的にコア内蔵するかは実装みて判断になると思いますが、PR 作成までは進める価値がありそうです。
上記の課題について方向性が決まり次第、PR 作成に着手します。
(追記: 方向性が既に固まっている機能モジュールについては、一括変更する PR を先行させます。e.g. #1419, #1420


もしMetasStoreManagerが不要であれば、実装するならtts_engine_manager.versions()一択・・・?

MetasStore(Managerではない既存のやつ)がコアを抱えるようになるため、.versions() を実装可能になります。
TTS 系と Metas 系は並列の立ち位置なので、どちらかに持たせるならどっちが良いか悩みそうです。

core_versions変数を作って、それをrouter側に渡す

現段階ではシンプルで良い方法だと思います。
ただこれをするとコア(音声ライブラリ)の動的追加が面倒になります。core_versions はリストかクラスか、リストなら実体をどこに置くか、実体の置き方ごとにリスト更新方法が異なるためバグりやすくなる等です。
一方で「音声ライブラリ機能が ON になるタイミングで再考すれば良い」という割り切りで core_versions 変数にするのも全然ありだと考えます。


core.supported_devices
...
... TTSEngineに実装できると嬉しい

👍️
こちらの方向性で実装します。

@tarepan tarepan added 状態:設計 設計をおこなっている状態 and removed 状態:必要性議論 必要性を議論している状態 labels Jun 23, 2024
@tarepan tarepan self-assigned this Jun 23, 2024
@tarepan tarepan added the 状態:実装 実装をおこなっている状態 label Jun 23, 2024
@tarepan tarepan changed the title refactor: TTSEngine を解体 refactor: コアを各機能モジュールに内蔵 Jun 23, 2024
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 23, 2024

MetasStore(Managerではない既存のやつ)がコアを抱えるようになるため、.versions() を実装可能になります。
TTS 系と Metas 系は並列の立ち位置なので、どちらかに持たせるならどっちが良いか悩みそうです。

あーーーなるほどです。

Engine側が良いかなと思いました!
過去バージョンがほしいのは音声合成なので、こっちかなと。

(気づいたのですが、もしかしたらMetasは最新のものだけで良いかも・・・?ちょっと考えきれてないですが)

core_versions変数を作って、それをrouter側に渡す

ただこれをするとコア(音声ライブラリ)の動的追加が面倒になります。core_versions はリストかクラスか、リストなら実体をどこに置くか、実体の置き方ごとにリスト更新方法が異なるためバグりやすくなる等です。
一方で「音声ライブラリ機能が ON になるタイミングで再考すれば良い」という割り切りで core_versions 変数にするのも全然ありだと考えます。

たーーーしかにです。
とりあえず疎結合に倒してcore_versionsにするのもありだなと僕も思いました。
まあどれも一長一短なので、.versions()でもcore_versionsでもどちらも良さそう・・・!

@tarepan tarepan removed 要議論 実行する前に議論が必要そうなもの 状態:設計 設計をおこなっている状態 labels Jun 28, 2024
@tarepan tarepan removed the 状態:実装 実装をおこなっている状態 label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants