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

__version__ とエンジンマニフェスト version の関係 #1232

Open
2 tasks done
tarepan opened this issue May 11, 2024 · 12 comments
Open
2 tasks done

__version__ とエンジンマニフェスト version の関係 #1232

tarepan opened this issue May 11, 2024 · 12 comments

Comments

@tarepan
Copy link
Contributor

tarepan commented May 11, 2024

質問の内容

現在の VOICEVOX ENGINE はエンジンバージョンを以下の 2 箇所で管理しているようにみえる:

  • voicevox_engine.__init____version__ 変数
  • engine_manifest.jsonversion

前者は /version API の返り値や FastAPI インスタンスの引数として利用されている。
後者は利用箇所がみつからなかった。

外部から注入される値は後者に書くのが本レポジトリの原則であるため、後者での一括管理が望ましいと考えうる。

このような背景から以下の質問があります:

  • 2箇所での管理は意図したものか、その場合の意図は何か
  • 前者の削除によるリファクタリングはOKか

VOICEVOXのバージョン

0.19.0

@tarepan tarepan added the 要議論 実行する前に議論が必要そうなもの label May 11, 2024
@Hiroshiba
Copy link
Member

manifest.jsonの方のバージョンはマニフェストのフォーマットバージョンを、__version__はそのエンジンのバージョンを意図しています!

manifest_version: str = Field(title="マニフェストのバージョン")

(フォーマットバージョンの方は普通に整数にしておけばよかったなとちょっと後悔しています・・・。)

@tarepan
Copy link
Contributor Author

tarepan commented May 16, 2024

齟齬がありそうです。私の意図は ↓ の感じです。

現在の engine_manifest.jsonmanifest_versionversion を定義しています。後者 (version) が上の質問における「engine_manifest.jsonversion」を指しています。

"manifest_version": "0.13.1",
"name": "DUMMY Engine",
"brand_name": "DUMMY",
"uuid": "c7b58856-bd56-4aa1-afb7-b8415f824b06",
"version": "999.999.999",

@Hiroshiba
Copy link
Member

ああ、なるほどです!すみません、勘違いしてました!

manifestの方はエンジン起動前に情報を得られるようにするためです!
サードパーティーPC アプリから利用する場合、エンジンを起動する前に何を起動しようとしてるかわかる必要があるので。

・・・マニフェストに書かれている方のバージョンをAPI でも 返す形で良さそうな気がしますね!!!
ビルドプロセスでコード内のversionとmanifest内のversionどちらも置き換えてそうだし、おっしゃる通りマニフェストファイルの方に書かれてる方で共通化するのが良さそうに思いました!

jq '.version = "${{ needs.config.outputs.version_or_latest }}"' engine_manifest.json > engine_manifest.json.tmp
mv -f engine_manifest.json.tmp engine_manifest.json
# Replace version & specify dynamic libraries
$sed -i "s/__version__ = \"latest\"/__version__ = \"${{ needs.config.outputs.version_or_latest }}\"/" voicevox_engine/__init__.py

あ。強いて言うならビルド時にバージョン埋め込んで、後から変更不可能にできてるのでそれ関連でメリットはあるかも・・・?
いやでも後から変えられるようにした方が、例えばキャラクターを追加した場合はリビルド不要だったりもするはずなので、manifest側のバージョンを返す方が便利そう・・・。

賛成寄りです!
(が、何か見落としがないかちょっと不安ではあります。)

@tarepan
Copy link
Contributor Author

tarepan commented May 21, 2024

manifestの方はエンジン起動前に情報を得られるようにするため

👍️
納得です。

マニフェストに書かれている方のバージョンをAPI でも 返す形で良さそう ... 何か見落としがないかちょっと不安

👍️
影響範囲が見えづらいため、一旦 engine_manifest.version 統一で draft 実装してみます。
そこで見えてきた問題点ベースで再考察し、Go/NoGo の最終結論を得られればと思います。

@Hiroshiba
Copy link
Member

@y-chan @sabonerune @takana-v メンションすみません!
ちょっと問題点がないか探しており、もし思いついたら指摘いただきたい感じです 🙇

今エンジンAPIが返すバージョンは、Pythonコード内に埋め込まれたバージョンを返す仕様なのですが、この仕様をmanifest.json内にあるエンジンバージョンを返す形に変えようとしています。
どちらもビルド時に↓で動的に書き換えている形なので、とりあえず普通の動作的には問題ないとは思うのですが、なにか問題点とか思いつかれたらコメントお願いできると・・・ 🙇

jq '.version = "${{ needs.config.outputs.version_or_latest }}"' engine_manifest.json > engine_manifest.json.tmp
mv -f engine_manifest.json.tmp engine_manifest.json
# Replace version & specify dynamic libraries
$sed -i "s/__version__ = \"latest\"/__version__ = \"${{ needs.config.outputs.version_or_latest }}\"/" voicevox_engine/__init__.py

@sabonerune
Copy link
Contributor

実用上は全く問題がないと思います。

個人的に気になったのはengine_manifest.jsonは設定ファイルではなく「エンジンの仕様をエンジンを起動せずに確認できるもの」なのでデータの流れる方向が逆に感じることです。

@Hiroshiba
Copy link
Member

@sabonerune ありがとうございます!

個人的に気になったのはengine_manifest.jsonは設定ファイルではなく「エンジンの仕様をエンジンを起動せずに確認できるもの」なのでデータの流れる方向が逆に感じることです。

確かに、言われてみれば僕も同じ違和感を一瞬感じました。
でもpackage.json`にライブラリ名やバージョンが書いていて、それをライブラリ内から読むのに似てると考えると、なんかさほど問題ではないというか、割とよくあることな気がしました。

・・・うーん。わからない。
冗長っちゃぁ冗長なのと、もうすでにマニフェストファイルから名前とか取得してページのレンダリングに使ってるので、別にいいのかもしれない。うーん。

@Hiroshiba
Copy link
Member

Hiroshiba commented May 24, 2024

実は2つで役割が違うかもしれない?
__version__に書いてる方はサーバーというライブラリみたいなもののバージョンで、マニフェストに書いてある方はエンジンそのもののバージョンみたいな。
だからまあ、極論言うとこれらは一致してなくても良い。
例えばまあ、サーバー側はAPI 構成が変わるたびに大きめのバージョンアップをして、エンジン側はキャラクターが追加されるたびに起きるのバージョンアップをするという運用をするのも許されそう。

と考えると、manifest.jsonにアクセスできる人(つまりPC 向けサードパーティアプリ開発者)はマニフェストを頼りにAPI を叩けるので、サーバー側のバージョン__version__は不要ですね!
一方でマニフェストにアクセスできない人はサーバーのバージョン(あるいは API ドキュメント)から判断するしかないので、サーバー側のバージョンが必要になるかもです。

と考えると、サーバーの/versionはマニフェストの中に書いてあるエンジンのバージョンではなく、__version__を返すべきな気がしてきました!
そしてマニフェストの中に書いてるバージョンはサードパーティアプリ向けなので、それはそれで提供する必要がある気がしました!

ということでどっちも必要という結論ですね!!

@tarepan
Copy link
Contributor Author

tarepan commented May 24, 2024

@Hiroshiba
回答が更新されたと認識しました。
以下の認識で合っているでしょうか?

意味 目的
__version__ サーバー(ライブラリ)バージョン1 API 越しにサーバー対応状況を取得2
.version エンジンバージョン3 エンジン起動前にファイルから情報を取得4

Q: 2箇所での管理は意図したものか、その場合の意図は何か
A: 意図したものである。異なる情報を異なる利用者へ提供するため。

Q: 前者の削除によるリファクタリングはOKか
A: 意図して使い分けられているので削除は不可。

Footnotes

  1. __version__に書いてる方はサーバーというライブラリみたいなもののバージョン from Hiho

  2. 一方でマニフェストにアクセスできない人はサーバーのバージョン(あるいは API ドキュメント)から判断するしかないので、サーバー側のバージョンが必要になる from Hiho

  3. マニフェストに書いてある方はエンジンそのもののバージョンみたいな。 from Hiho

  4. manifestの方はエンジン起動前に情報を得られるようにするため from Hiho

@Hiroshiba
Copy link
Member

ですね!!

そしてこれは守られない気がしてきました!!
ややこしいというのと、仮にforkしてオリジナルのものを作ってビルドすると、元のVOICEVOXサーバーバージョンに関係なくどちらにも同じ値が入ってしまうなぁと。
だからちゃんとやるならVOICEVOX OSS ENGINEがバージョンアップする事に__version__をインクリメントしていく必要があるけど、ちょっとだけ大変。

あとサーバー対応状況は一応APIからでも/engine_manifestで得られることに気づきました。

・・・・・・・・・・・・・/versionはエンジンバージョンで良い気がしました!!
APIサーバーのバージョンは取得できず、どんなAPIがあるかは/engine_manifestから得てもらう形で。
もし必要になったら/api_versionを生やすと良さそう。

二転三転していてすみません。。。 🙇

@tarepan
Copy link
Contributor Author

tarepan commented May 27, 2024

これは守られない気が

👍️
同意です。
メリットはあり豪華な設計ですが、運用コストに見合うメリットは得られないと考えます。


以下で認識合っているでしょうか?

暗示的仕様を変更し「『__version__』と『engine_manifest.json.version』は共にエンジンバージョンを指す」とする。
エンジンバージョンのアクセス方法は「エンジン起動前の engine_manifest.json 直接読み込み」「GET /version API アクセス」の二通りある。

上記の認識のうえで、使い分けが無くなったので __version__ 削除によるリファクタリングは可能になった、という理解で良いでしょうか?

@Hiroshiba
Copy link
Member

Hiroshiba commented May 28, 2024

はい、そんな感じかなと!!

__version__はややこしいので無いほうがよいと思います!


追記:あ、ちょっと違うかもです!

「『API /version』と『engine_manifest.json.version』は共にエンジンバージョンを指す」
という認識です。

__version__は・・・まあ今まではエンジンバージョンが刻まれていたけど、これからは未定義という感じでしょうか!

@tarepan tarepan removed the 要議論 実行する前に議論が必要そうなもの label May 29, 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

No branches or pull requests

3 participants