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.

Sign up for GitHub

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

Jump to bottom

feat!: voicevox_vvmからVVMをダウンロードする #928

Merged
merged 7 commits into from
Jan 23, 2025
Merged

feat!: voicevox_vvmからVVMをダウンロードする #928

merged 7 commits into from
Jan 23, 2025

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 19, 2025

内容

BREAKING-CHANGE: modelsのダウンロード元をvoicevox_vvmに。
BREAKING-CHANGE: modelsのダウンロード先のディレクトリ名を"model"から"models"に。

関連 Issue

Refs: #825

その他

必要があるのなら、このPRの前にリファクタPR等を挟み込みます。

また、ドキュメントには手を付けないままにしました。というのも既にこういう形でしか動かせなくなってるので…

❯ cargo run -p downloader -- --core-repo qryxip/voicevox_core --version 999.999.999 --onnxruntime-version voicevox_onnxruntime-999.999.999

@qryxip qryxip requested a review from Hiroshiba January 19, 2025 07:21
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Jan 19, 2025
@qryxip
refactor: VOICEVOX#928 用のコード移動
02f1161
@qryxip
Copy link
Member Author

qryxip commented Jan 19, 2025 •
edited
Loading

必要があるのなら、このPRの前にリファクタPR等を挟み込みます。

#929 を作りました。

[追記] #929 との差分はこれで確認できます。
github.com/qryxip/voicevox_core/compare/feat-download-vvms-from-voicevox-vvm...refactor-code-movement-for-pr928
https://github.com/qryxip/voicevox_core/compare/refactor-code-movement-for-pr928..feat-download-vvms-from-voicevox-vvm

@qryxip qryxip marked this pull request as draft January 19, 2025 07:27
qryxip
qryxip commented Jan 19, 2025
View reviewed changes
crates/downloader/src/main.rs Outdated
Comment on lines 51 to 52
static ALLOWED_MODELS_VERSIONS: LazyLock<VersionReq> =
LazyLock::new(|| "=0.0.1-preview.0".parse().unwrap());
Copy link
Member Author

Choose a reason for hiding this comment

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

よく考えたら0.0.1-preview.0 → 0.0.1-preview.1で非互換性を突っ込みたくならない保証は無いよなと思ってとりあえず。

Copy link
Member

Choose a reason for hiding this comment

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

なんかこれ変えるの忘れそうですね･･･！！

Copy link
Member Author

Choose a reason for hiding this comment

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

どれが選ばれたのかは表示するようにしているので、0.0.1-preview.0が選ばれ続けるのは気付くようにはなってるかなーとは思いました。一応。

qryxip added a commit that referenced this pull request Jan 19, 2025
@qryxip
refactor: #928 用のコード移動 (#929)
b20f8b6
@qryxip qryxip marked this pull request as ready for review January 19, 2025 15:45
@qryxip qryxip requested review from Hiroshiba and removed request for Hiroshiba January 20, 2025 15:33
Hiroshiba
Hiroshiba approved these changes Jan 20, 2025
View reviewed changes
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！！！

voicevox_core/models/vvms/以下にダウンロードされるっぽいけど、
voicevox_core/models/以下でも良いかも？

細かいこと書いてますが、たぶん問題ないと思うのでマージいただければ！！

crates/downloader/src/main.rs Outdated Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Jan 22, 2025

af57c58 (#928): terms.mdに加え、READMEもダウンロードに入れるようにしました。

Hiroshiba
Hiroshiba reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

READMEをダウンロードするの、良さそう！
ちなみにREADMEをダウンロードしたい意図ってなんでしょう？　スタイルのテーブルがあること？

ディレクトリ構造ですが、今terms.mdをmodels/vvms/terms.mdにダウンロードしているの、発見しやすいようにmodels/terms.mdに配置するのどうでしょう？
voicevox_vvmリポジトリ側もそうしてしまって、コアも合わせる形でもOKです！

@qryxip
Copy link
Member Author

qryxip commented Jan 22, 2025

ちなみにREADMEをダウンロードしたい意図ってなんでしょう？　スタイルのテーブルがあること？

スタイルのテーブルもありますが、現状の他 (CORE本体やONNX Runtime等)に合わせた形ですね。まあ必須ではないといえばないかも。

models/terms.md

賛成です。

voicevox_vvmリポジトリ側もそうしてしまって、コアも合わせる形でもOKです！

そっちの方がよさそうですね。一貫性のために。

@Hiroshiba
Copy link
Member

あっそうか他もreadme落とすのか。
でしたらたしかに揃えた方が綺麗そう！！

リポジトリの方のファイルパス、あとで変えておきます！！

@Hiroshiba Hiroshiba mentioned this pull request Jan 22, 2025
Merged
@Hiroshiba
Copy link
Member

ファイルパスを変更してreleaseも作成しました！

@qryxip
Copy link
Member Author

qryxip commented Jan 23, 2025

3733cec (#928): 0.0.1-preview.1を使うようにしました。

qryxip
qryxip commented Jan 23, 2025
View reviewed changes
crates/downloader/src/main.rs Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 23, 2025
View reviewed changes
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.

approve変わらずです！
再度approveがほしいなどあればreviewのre-requestいただければ！

@qryxip qryxip merged commit 40f1e3b into VOICEVOX:main Jan 23, 2025
34 checks passed
@qryxip qryxip deleted the feat-download-vvms-from-voicevox-vvm branch January 23, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qryxip @Hiroshiba