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

C API用のzipファイルからmodelディレクトリをなくし、別でmodelダウンロード可能にする #603

Merged

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Sep 9, 2023

内容

タイトルの通り、C API用のzipファイルからmodelディレクトリをなくし、別でmodelダウンロード可能にします。

ビルド時にFAT RESOURCEやサンプルのmodelディレクトリを導入させず、別でそのディレクトリのZIPファイルを作成してアップロードしています。
ダウンロードではminを指定していない場合に辞書ファイルなどと一緒にモデルディレクトリがダウンロードされる感じです。

関連 Issue

fix #596

その他

サンプルでのビルド結果こちらです。
https://github.com/Hiroshiba/voicevox_core/releases/tag/0.15.0-checkmodelsplit.16

の変更が含まれているので先にそちらをマージする必要があるかもです。

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Sep 9, 2023

ダウンローダーを変更するとダウンローダーのテストが実行されるのですが、releasesのlatestのファイルを見に行くのでmodel.zipが存在せずテストが落ちてしまいそうです。
プッシュ時にダウンローダーをチェックできるのはビルドまでで、テストが実行できるのはリリースした時だけな気がしました。
少なくともプッシュ時のダウンローダーの実行テストは不可能なのでやめて、デプロイ後にテスト実行するのが良さそうに思います。

でもテストが多すぎる問題を解決したプルリクエストでワークフローを分けた関係で、リリース後にダウンロードのテストをするのが難しくなっています(2つのワークフローが完了した後に実行しないといけない)。
片方のワークフローからもう片方実行すればいいのですが、↓のコメントの通りやりづらくなってます。
#560 (comment)

解決策としては、workflow_dispatchworkflow_callの両方を定義しようとするから↑の問題が発生するので、workflow_dispatchをやめてしまうのが良さそうです。

README.md Outdated Show resolved Hide resolved
crates/download/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 151 to 154
let model = find_gh_asset(octocrab, CORE_REPO_NAME, &version, |tag| {
format!("model-{tag}.zip")
})
.await?;
Copy link
Member

Choose a reason for hiding this comment

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

find_gh_assetfind_gh_assetsにすればアクセス回数を一つ減らせそうではありますね。以下のようにまですべきかどうかはともかくとして。

-    let core = find_gh_asset(octocrab, CORE_REPO_NAME, &version, |tag| {
+    let [core, model] = find_gh_assets(octocrab, CORE_REPO_NAME, &version, |tag| {
         let device = match (os, device) {
             (Os::Linux, Device::Cuda) => "gpu",
             (_, device) => device.into(),
         };
-        format!("{CORE_REPO_NAME}-{os}-{cpu_arch}-{device}-{tag}.zip")
-    })
-    .await?;
-
-    let model = find_gh_asset(octocrab, CORE_REPO_NAME, &version, |tag| {
-        format!("model-{tag}.zip")
+        [
+            format!("{CORE_REPO_NAME}-{os}-{cpu_arch}-{device}-{tag}.zip"),
+            format!("model-{tag}.zip"),
+        ]
     })
     .await?;

     let additional_libraries = OptionFuture::from((device != Device::Cpu).then(|| {
-        find_gh_asset(
+        find_gh_assets(
             octocrab,
             ADDITIONAL_LIBRARIES_REPO_NAME,
             &additional_libraries_version,
             |_| {
                 let device = match device {
                     Device::Cpu => unreachable!(),
                     Device::Cuda => "CUDA",
                     Device::Directml => "DirectML",
                 };
-                format!("{device}-{os}-{cpu_arch}.zip")
+                [format!("{device}-{os}-{cpu_arch}.zip")]
             },
         )
     }))
     .await
-    .transpose()?;
+    .transpose()?
+    .map(|[asset]| asset);
-async fn find_gh_asset(
+async fn find_gh_assets<const N: usize>(
     octocrab: &Arc<Octocrab>,
     repo: &str,
     git_tag_or_latest: &str,
-    asset_name: impl FnOnce(&str) -> String,
-) -> anyhow::Result<GhAsset> {
+    asset_names: impl FnOnce(&str) -> [String; N],
+) -> anyhow::Result<[GhAsset; N]> {
     let Release {
         html_url,
         tag_name,
         assets,
         ..
     } = {
         let repos = octocrab.repos(ORGANIZATION_NAME, repo);
         let releases = repos.releases();
         match git_tag_or_latest {
             "latest" => releases.get_latest().await,
             tag => releases.get_by_tag(tag).await,
         }?
     };

-    let asset_name = asset_name(&tag_name);
-    let Asset { id, name, size, .. } = assets
-        .into_iter()
-        .find(|Asset { name, .. }| *name == asset_name)
-        .with_context(|| format!("Could not find {asset_name:?} in {html_url}"))?;
+    return asset_names(&tag_name).try_map_(|asset_name| {
+        let Asset { id, name, size, .. } = assets
+            .iter()
+            .find(|Asset { name, .. }| *name == asset_name)
+            .with_context(|| format!("Could not find {asset_name:?} in {html_url}"))?;
+
+        Ok(GhAsset {
+            octocrab: octocrab.clone(),
+            repo: repo.to_owned(),
+            tag: tag_name.clone(),
+            id: *id,
+            name: name.clone(),
+            size: *size as _,
+        })
+    });

-    Ok(GhAsset {
-        octocrab: octocrab.clone(),
-        repo: repo.to_owned(),
-        tag: tag_name,
-        id,
-        name,
-        size: size as _,
-    })
+    #[easy_ext::ext]
+    impl<T, const N: usize> [T; N] {
+        fn try_map_<F, O, E>(self, f: F) -> Result<[O; N], E>
+        where
+            F: FnMut(T) -> Result<O, E>,
+        {
+            let vec = self.into_iter().map(f).collect::<Result<Vec<_>, _>>()?;
+            Ok(vec
+                .try_into()
+                .unwrap_or_else(|_| unreachable!("the lengths should be same")))
+        }
+    }
 }

Copy link
Member Author

@Hiroshiba Hiroshiba Oct 12, 2023

Choose a reason for hiding this comment

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

すみません、まとめた方が良さそうというのはわかるのですが、ちょっと難しすぎそうなのでパスさせてください 🙇

@Hiroshiba
Copy link
Member Author

レビューありがとうございます!!ドラフト状態なので他のプルリクエストが完了次第反映できればと思います!

@Hiroshiba Hiroshiba closed this Oct 11, 2023
@Hiroshiba Hiroshiba deleted the コアのmodelディレクトリを分離 branch October 11, 2023 18:19
@Hiroshiba Hiroshiba restored the コアのmodelディレクトリを分離 branch October 11, 2023 18:19
@Hiroshiba Hiroshiba reopened this Oct 11, 2023
@Hiroshiba Hiroshiba marked this pull request as ready for review October 12, 2023 17:54
@Hiroshiba
Copy link
Member Author

ビルドが通ったのでドラフト開けました!

@Hiroshiba
Copy link
Member Author

ビルドワークフローの実行結果です。

下がダウンローダー、上がライブラリのビルドです。
ライブラリのビルド後のテストに落ちてますが、これはダウンローダーがリポジトリ指定できずVOICEVOX Orgのデータを取ってこようとするのでエラーになる形です 😇

crates/download/src/main.rs Outdated Show resolved Hide resolved
.github/workflows/download_test.yml Outdated Show resolved Hide resolved
@@ -255,7 +267,7 @@ fn octocrab() -> octocrab::Result<Arc<Octocrab>> {

async fn find_gh_asset(
octocrab: &Arc<Octocrab>,
repo: RepoName,
repo: &RepoName,
Copy link
Member

@qryxip qryxip Oct 13, 2023

Choose a reason for hiding this comment

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

ここをrepo: RepoNameにしたのはC-CALLER-CONTROLに従った結果だったのですが、まあどちらでも良いと思います。

こういう考え方も主流ですし(というか私も普段書き殴るときはどちらかというとこっち側です)
https://twitter.com/qnighy/status/1008010536397651968

Copy link
Member Author

Choose a reason for hiding this comment

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

あんま深いこと考えてなくて、find_gh_assetを2回呼ぶのでrepoが移譲?されちゃって以降で使えなくなっちゃったんで&つけた感じでした!

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

LGTM!

@Hiroshiba
Copy link
Member Author

ビルドワークフローが通りました!!!!! ありがとうございました!!!!
https://github.com/Hiroshiba/voicevox_core/actions/runs/6510071311/job/17683513298

リリースこちら
https://github.com/Hiroshiba/voicevox_core/releases/tag/0.15.0-checkmodelsplit.16

@Hiroshiba Hiroshiba merged commit bd7f4db into VOICEVOX:main Oct 13, 2023
36 of 44 checks passed
@Hiroshiba Hiroshiba deleted the コアのmodelディレクトリを分離 branch October 13, 2023 15:53
@Hiroshiba
Copy link
Member Author

0.15.0-preview.12 ビルドを開始しました!

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