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

feat: vulkan backend #168

Closed
wants to merge 4 commits into from
Closed

feat: vulkan backend #168

wants to merge 4 commits into from

Conversation

newfla
Copy link
Contributor

@newfla newfla commented Aug 6, 2024

The PR introduces/modifies:

  • Update whisper.cpp to the latest master commit (at time of writing a new version has not been released yet)
  • Refactor build.rs to reflect deprecation of WHISPER_XYZ cmake variables and changes to build artifacts folders
  • Minor adjustments to binding.rs and WhisperGrammarElementType repr on Windows
  • Introduce vulkan support (tested on linux and windows)

Unfortunately I can't test on macos. I would be very happy if someone could give me some feedback on that platform.

arch-vulkan-test

@arizhih
Copy link
Contributor

arizhih commented Aug 9, 2024

@newfla
Hi, I've also created similar PR #169, I don't want to mix adding new features with updating to the new whisper.cpp cause there are a lot of changes. So I just added support for new project structure and it looks like it works.

As for your PR there is some issues despite all tests are passed. If you look into details - bindings not generated and it uses old, but some structures from public API were changed e.g. speed_up was removed from whisper_full_params, but it exists in our bindings.

So we can split our efforts and leave one PR with updating to the new whisper.cpp version and another one with adding vulkan support. Or you can add missed parts to your PR, feel free to use my PR as reference.

@tazz4843 Hi, please notice that all tests are passed, but really we have problems. What do you think about to disable implicit use of the bundled bindings at all or at least in CI runs?

@newfla
Copy link
Contributor Author

newfla commented Aug 10, 2024

@arizhih
Hi, I will wait your pr to be merged to rebase this branch on top of your work.
In the meantime I'm converting this pr to a draft so that it can't be merged by mistake before yours.

@thewh1teagle
Copy link
Contributor

thewh1teagle commented Aug 13, 2024

@newfla
Did you managed to build whisper.cpp with vulkan on WIndows?

I tried with whisper.cpp directly but it failed with error.

I used the following commands:

# Install vulkan from https://vulkan.lunarg.com/
git clone https://github.com/ggerganov/whisper.cpp
cd whisper.cpp
$env:VULKAN_SDK = "C:\VulkanSDK\1.3.290.0"
cmake -B build . -DGGML_VULKAN=ON -DGGML_CCACHE=OFF

And got this error:

-- Found Vulkan: C:/VulkanSDK/1.3.290.0/Lib/vulkan-1.lib (found version "1.3.290") found components: glslc glslangValidator
-- Vulkan found
CMake Error at ggml/src/CMakeLists.txt:613 (add_subdirectory):
  The source directory

    D:/whisper.cpp/ggml/src/vulkan-shaders

  does not contain a CMakeLists.txt file.

Looks like the build with volkan is a bit more complicated in WIndows:
https://github.com/ggerganov/llama.cpp/blob/master/docs/build.md#vulkan

@newfla
Copy link
Contributor Author

newfla commented Aug 14, 2024

@thewh1teagle windows msvc is fine with this branch without doing anything special.
VulkanSDK-1.3.290.0, AMD Adrenalin 24.7.1, R5 7530U
vulkansdk
terminal output
gpu stats

@arizhih
Copy link
Contributor

arizhih commented Aug 14, 2024

@thewh1teagle I can confirm that with this commit ggerganov/whisper.cpp@fe36c90 (used in pr) you can build whisper.cpp with Vulkan, unfortunately master is broken right now

@thewh1teagle
Copy link
Contributor

thewh1teagle commented Aug 15, 2024

Thanks. the specific commit works. Looks like master is broken as you said.
I did a bit of tests. It utilize the GPU by 80% and transcribe. But comparing to OpenCL seems like it's a bit slower.

By the way I couldn't enable openblas when compile

@arizhih
Copy link
Contributor

arizhih commented Sep 13, 2024

@newfla
Hi, my PR was merged some time ago, do you have any plans to continue work on this PR?

@newfla
Copy link
Contributor Author

newfla commented Sep 13, 2024

@newfla Hi, my PR was merged some time ago, do you have any plans to continue work on this PR?

@arizhih
Hi, from what I see in #170 @thewh1teagle has already started the merge process.
If you agree I would say to continue with #170

@tazz4843
Copy link
Owner

In that case closing in favor of 170

@tazz4843 tazz4843 closed this Sep 13, 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

Successfully merging this pull request may close these issues.

4 participants