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

voicevox: 0.20.0 -> 0.22.3; voicevox-engine: 0.20.0 -> 0.22.2; voicevox-core: 0.15.4 -> 0.15.7 #367083

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Dec 21, 2024

Followup to #319403

This PR bumps all the voicevox package versions to the latest non-pre-release version.

I also changed over to using fetchurl instead of fetchzip for voicevox-core. fetchurl won't uncompress the zip file, so it takes up less space (though the difference is not too big).
This will also facilitate the creation of an update script, since we won't need to unpack to get the hash.
(though I probably won't make an update script, since it's much easier to update manually)

I also appended ${version} to voicevox-engine.passthru.resources's derivation name. This way when we want to update voicevox-engine, but forget to update the hash of the resources to, it will throw a hash mismatch error.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@TomaSajt
Copy link
Contributor Author

@ofborg build voicevox-core.src

@TomaSajt TomaSajt marked this pull request as draft December 21, 2024 12:38
@TomaSajt
Copy link
Contributor Author

@ofborg build voicevox

@TomaSajt
Copy link
Contributor Author

image
When temporarily setting free = true it seemed to build on all platforms (didn't wait out aarch64-darwin)

@TomaSajt TomaSajt marked this pull request as ready for review December 21, 2024 20:23
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Dec 21, 2024
@eljamm eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 23, 2024
@TomaSajt TomaSajt changed the title voicevox-core: 0.15.4 -> 0.15.5; voicevox{,-engine}: 0.20.0 -> 0.21.1 voicevox: 0.20.0 -> 0.22.1; voicevox-engine: 0.20.0 -> 0.22.0; voicevox-core: 0.15.4 -> 0.15.5 Dec 25, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 26, 2024
Copy link
Contributor

@eljamm eljamm left a comment

Choose a reason for hiding this comment

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

I tried to run nixpkgs-review but triton-llvm is taking a lot of time and resources to build so I just gave up on that. I did test the update, though, and the program seems to be working fine.

Also, @TomaSajt, you can add me as a maintainer if you'd like someone to co-maintain this with. Else, feel free to request me for review.

@eljamm eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 26, 2024
@TomaSajt
Copy link
Contributor Author

@eljamm I added you as a maintainer

Copy link
Contributor

@eljamm eljamm left a comment

Choose a reason for hiding this comment

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

Thanks! Re-approving since there was a force-push.

@TomaSajt
Copy link
Contributor Author

I tried to run nixpkgs-review but triton-llvm is taking a lot of time and resources to build so I just gave up on that.

Do you know how triton-llvm is possibly related to the changes in this PR?

@eljamm
Copy link
Contributor

eljamm commented Dec 26, 2024

Here is the dependency graph from nom:

┏━ Dependency Graph:
┃    ┌─ ⏸ hardcode-paths.patch waiting for 2 ⏵
┃ ┌─ ⏸ voicevox-0.22.1
┃ │              ┌─ ⏵ triton-llvm-19.1.0-rc1 (configurePhase) ⏱ 10s
┃ │           ┌─ ⏸ python3.12-triton-3.1.0
┃ │        ┌─ ⏸ python3.12-torch-2.5.1
┃ │        ├─ ⏵ python3.12-jax-0.4.28 (pytestCheckPhase) ⏱ 10s
┃ │     ┌─ ⏸ python3.12-nanobind-2.4.0
┃ │  ┌─ ⏸ python3.12-soxr-0.5.0.post1
┃ ├─ ⏸ voicevox-engine-0.22.0
┃ ⏸ review-shell

When I run nix-build . -A voicevox everything builds fine, but I have to build triton-llvm with nixpkgs-review.

I assume this is happening because python3.12-soxr or its deps changed in master and is not cached, right?

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Dec 26, 2024

I did some manual bisecting and looks like there was a staging-next merge recently and I think that caused it to start rebuilding. (maybe it doesn't build, so it didn't get cached)
I'll look into it.

@TomaSajt
Copy link
Contributor Author

Looks like some weird test failure making it not build
https://hydra.nixos.org/build/281892957/nixlog/5

Other people have already reported this issue:
#363965
#367784

@ofborg ofborg bot requested a review from eljamm December 26, 2024 18:49
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Dec 29, 2024
@TomaSajt
Copy link
Contributor Author

Haven't rebased, since triton-llvm has only just been fixed, so it isn't cached yet.

Added a commit that makes it easier to update the voicevox-core artifact hashes in the future.

@TomaSajt TomaSajt force-pushed the voicevox branch 2 times, most recently from e7dc4f9 to 745aebb Compare December 29, 2024 22:46
@TomaSajt
Copy link
Contributor Author

triton-llvm is now cached, so I was able to rebase on latest master.
I had to disable some tests: it was some minor floating-point imprecision issue, not sure why it only showed up now.

@eljamm eljamm removed the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Dec 29, 2024
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Dec 30, 2024

Looks like torch is still broken on darwin, so it won't build there.
In any case the app built successfully on both linux platforms

@eljamm
Copy link
Contributor

eljamm commented Dec 30, 2024

Would you rather mark voicevox as broken on darwin and merge or wait until the issue is resolved?

@TomaSajt
Copy link
Contributor Author

Would you rather mark voicevox as broken on darwin and merge or wait until the issue is resolved?

As far as I know it, it will work once torch gets fixed, so we don't need to mark this package as broken.
Meaning we can merge whenever.

Copy link
Contributor

@eljamm eljamm left a comment

Choose a reason for hiding this comment

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

Alright. This LGTM, then.

@eljamm eljamm added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Dec 30, 2024
@ofborg ofborg bot requested a review from eljamm December 30, 2024 07:13
@eljamm eljamm removed 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Dec 30, 2024
@eljamm
Copy link
Contributor

eljamm commented Jan 2, 2025

According to #356030, tensorflow-bin (>2.16) is no longer supported for x86_64-darwin, so in turn nanobind, soxr and finally voicevox-engine fail to build. I think it might be best to drop x86_64-darwin support until a decision is made to either drop or pin tensorflow-bin.

That said, nanobind also fails on aarch64-darwin, but the issue is not clear for me as its tests apparently pass.

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 2, 2025

According to #356030, tensorflow-bin (>2.16) is no longer supported for x86_64-darwin, so in turn nanobind, soxr and finally voicevox-engine fail to build. I think it might be best to drop x86_64-darwin support until a decision is made to either drop or pin tensorflow-bin.

That said, nanobind also fails on aarch64-darwin, but the issue is not clear for me as its tests apparently pass.

How is tensorflow-bin related? It's not in the build closure for me on x86_64-darwin.
The thing that is failing currently is just torch, but that only seems like an LLVM bump issue.

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 2, 2025

++ lib.optionals (!(builtins.elem stdenv.hostPlatform.system tensorflow-bin.meta.badPlatforms)) [
tensorflow-bin
jax
jaxlib
];

Oh wow, interesting

@TomaSajt TomaSajt requested a review from drupol January 2, 2025 17:23
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 2, 2025

@drupol could you take a quick look at this PR? It's pretty similar to the recent spotube PR.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Diff LGTM

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 2, 2025
@TomaSajt TomaSajt merged commit e238f8a into NixOS:master Jan 2, 2025
22 of 24 checks passed
@eljamm
Copy link
Contributor

eljamm commented Jan 2, 2025

I know that we said we can merge whenever and that we don't need to mark the package as broken, but I'm curious about why we did so when x86_64-darwin won't work. Is there something that I'm missing, perhaps?

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 2, 2025

It will work, once torch gets fixed.

@eljamm
Copy link
Contributor

eljamm commented Jan 2, 2025

You're right, my bad. I wasn't aware that x86_64-darwin was already in tensorflow-bin.meta.badPlatforms and is being skipped, even though you posted that earlier. I'm a bit confused about why I though this was an issue originally, but I guess that doesn't matter, now 😅

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 5, 2025

btw just checked now that torch is fixed, and darwin seems to function OK.

@eljamm
Copy link
Contributor

eljamm commented Jan 5, 2025

Indeed, thanks a lot for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants