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

add: voicevox_onnxruntimeをビルドできるようにする #52

Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Aug 30, 2024

内容

voicevox_onnxruntimeをビルドできるようにします。

inputs.target = "onnxruntime" | "voicevox_onnxruntime"でビルド対象を選び、"voicevox_onnxruntime"の場合次のsecretsを用いたビルドとなります。

  • ${{ secrets.PRODUCTION_GITHUB_TOKEN }}: 現行COREのと同じ役割
  • ${{ secrets.PRODUCTION_REPOSITORY_URL }}: 現行COREのと同じ役割
  • ${{ secrets.PRODUCTION_DISCORD_WEBHOOK }}: 失敗時にビルドログを送信(個人で動作確認済)

対象タグはv{バージョン}-voicevoxで固定です。

関連 Issue

ref VOICEVOX/voicevox_project#24

スクリーンショット・動画など

その他

@qryxip qryxip marked this pull request as ready for review August 31, 2024 08:24
@qryxip qryxip requested a review from Hiroshiba August 31, 2024 08:24
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です!!!

楽しみですね!!!!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -33,26 +40,28 @@ jobs:
matrix:
# TODO: 外せる`--compile_no_warning_as_error`は外す
include:
- artifact_name: onnxruntime-win-x64
- artifact_name: ${{ inputs.name || 'onnxruntime' }}-win-x64
Copy link
Member

Choose a reason for hiding this comment

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

(提案とよりはこんな感じの書き方もあるよというコメントです)

これ、config用のjob作ってそこで計算結果のnameを出力すれば、needs.config.outputs.nameで使えるようになるんですよね・・・。
そっちの方が一応3文字短い。
ただまぁ変数定義がめんどくさい(↓こんな感じ)し、各jobで needs: [config]がいるなどの変更も必要なのですが・・・。
https://github.com/VOICEVOX/voicevox_engine/blob/378b51e94da13974b59849cc47e3f9156c8bd678/.github/workflows/build-engine-package.yml#L36-L48

まあこのままでも今は良さそう!
将来もしもう1種類こういうのが増えたらさすがにconfig job書いた方が良さそう。

Copy link
Member Author

@qryxip qryxip Sep 2, 2024

Choose a reason for hiding this comment

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

リリースタグ名 (e.g. voicevox_onnxruntime-1.17.3)も共通化できるものの一つになりますね。このPRでもやらかしが一つありますし。うーん、今やっちゃいます?
(追記) まあこれはenvでも十分ではあるのですが

Copy link
Member

Choose a reason for hiding this comment

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

ほんとにどちらでもという気はします。

あ、でも少なくともまとめといたほうが良さそうな気がします。規模的にコピペは辛くなってきてそう。
envでも良いけど、経験上あとあとenvじゃダメなとこでも使いたくなってくるんですよねぇ。

決めづらい!
考えかたとして「1回書いてみる」でモチベでそうなら書いてみる、出ないなら今回はenvにしてみる、とかどうでしょ。

Copy link
Member Author

Choose a reason for hiding this comment

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

ですね。一旦envで…と思いましたが、これの$ONNXRUNTIME_NAME-$ONNXRUNTIME_VERSIONはちょっと辛いかも。ONNXRUNTIME_NAMEはともかく1.17.3の部分の記述は一箇所にしたい気持ちがあります。

env:
  ONNXRUNTIME_NAME:
    |-
    ${{ inputs.target || 'onnxruntime' }}
  ONNXRUNTIME_VERSION:
    |- # workflow_dispatchでのバージョン名が入る。無指定なら適当なバージョン
    ${{ inputs.version || '1.17.3' }}

Copy link
Member

Choose a reason for hiding this comment

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

ちょっと意図汲み取れてないかもですが、envでもOKだと思います!
気が向いたらconfigジョブでも。

Copy link
Member Author

@qryxip qryxip Sep 3, 2024

Choose a reason for hiding this comment

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

あ、一応意図を補足すると、envからenvは参照できないので定義の方法がこうなり、(他の部分はともかく)1.17.3が二度表われるというのがどうなのかという感じです。

env:
  ONNXRUNTIME_NAME:
    |-
    ${{ inputs.target || 'onnxruntime' }}
  ONNXRUNTIME_VERSION:
    |- # workflow_dispatchでのバージョン名が入る。無指定なら適当なバージョン
    ${{ inputs.version || '1.17.3' }}
  RELEASE_TAG:
    |-
    ${{ inputs.release && format('{0}-{1}', inputs.target || 'onnxruntime', inputs.version || '1.17.3') || '' }}
  #                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^
  #                    =env.ONXRUNTIME_NAME (多分`|| 'onnxruntime'`は不要)  =env.ONNXRUNTIME_VERSION

Copy link
Member

Choose a reason for hiding this comment

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

あーなるほどです!
config jobならシェルスクリプトが走らせられるので1回定義にできたり、1.17.3の方だけenvに書いたりとかはできそうですね。

run: |
(
git remote add private "$PRODUCTION_REPOSITORY_URL"
git fetch private "refs/tags/v$ONNXRUNTIME_VERSION-voicevox"
Copy link
Member

Choose a reason for hiding this comment

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

(判断メモ)

コアの時はタグ名を指定できるようにしてましたが、タグ名がバージョンに依存した固定の名前でもとりあえず良さそう!

もしタグ名を指定できるようにしておけば、onnxruntimeのバージョンが変わっても、製品版側のバージョンを更新する必要はない時にさくっとアプデをビルドできるかも。
けどまあ運用してみないと分からないので、どっちでも良さそう!

Copy link
Member Author

@qryxip qryxip Sep 2, 2024

Choose a reason for hiding this comment

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

後でvoicevox_onnxruntimeの本体部分もレビューして頂く流れになると思うのですが、minor version update (e.g. 1.19 → 1.20)レベルの変更だとコンフリクトが高頻度で発生するという可能性もあるかなと思ってます。まあ運用してみないとわからないというのは同意です。

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@qryxip
Copy link
Member Author

qryxip commented Sep 2, 2024

44b26a6 (#52)
voicevox_onnxruntimeにVOICEVOX_LICENSE.mdを追加しました。voicevox_onnxruntime側で別途レビューして頂く流れにはなりますが、内容は現行製品版COREのreadmeから持って来たものです。このonnxruntime-builderでのリリース時に次のようにリネームします。

voicevox_onnxruntimeのリポジトリ リリース
VOICEVOX_LICENSE.md LICENSE.md
LICENSE ORT_LICENSE
README.md ORT_README.md
ThirdPartyNotices.txt OrtThirdPartyNotices.txt

@qryxip
Copy link
Member Author

qryxip commented Sep 2, 2024

  • 7ce9d7a (#52): リリースノートに含める<table>のリンク先が壊れてしまったため修正
  • e5e96ae (#52): git mergeではなくgit switch -dにしたので-c user.name=dummy -c [email protected]は不要なはず。なので削除

@Hiroshiba
Copy link
Member

voicevox_onnxruntimeにVOICEVOX_LICENSE.mdを追加しました。voicevox_onnxruntime側で別途レビューして頂く流れにはなりますが、内容は現行製品版COREのreadmeから持って来たものです。このonnxruntime-builderでのリリース時に次のようにリネームします。

おお!!ありがたいです!!
最終的な配布物のどこに何を含めていくかでまた調整するかもですが、少なくともこのリポジトリの成果物はいったん良さそう!

ちょっと別のコード管理方法も思いついたので、該当箇所にコメントします 🙏

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!!!

ONNXRUNTIME_NAMEのとこはすみません、一旦おまかせということで。。。 🙇

@qryxip
Copy link
Member Author

qryxip commented Sep 4, 2024

2a15805 (#52): #53 でPrivacy.mdを消すようにしたので対応

@Hiroshiba
Copy link
Member

マージします!

@Hiroshiba Hiroshiba merged commit fbf8847 into VOICEVOX:main Sep 4, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

2 participants