Skip to content

ci : enable bindings java job #3070

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

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

danbev
Copy link
Member

@danbev danbev commented Apr 24, 2025

This pull requests re-enables the java binding job which has been disabled.

danbev added 2 commits April 25, 2025 09:39
This commit re-enables the job previously name `java` which was
disabled in the build.yml file.

The motivation for this is that we recently fixed a few issue in the
java bindings and it should be possible to build them on windows.

Refs: ggml-org#2949
Refs: ggml-org#2781
@danbev danbev force-pushed the ci-enable-bindings-java-job branch from 76b109e to f172fd0 Compare April 25, 2025 07:45
@danbev danbev marked this pull request as ready for review April 25, 2025 07:50
@@ -135,6 +135,22 @@ if (NOT TARGET ggml)
add_library(ggml ALIAS ggml::ggml)
else()
add_subdirectory(ggml)
if(WIN32)
# The following adds a _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR macro and is a workaround for
Copy link
Member Author

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.

@ggerganov Does this look acceptable as a workaround for this? With this the bindings-java job is able to pass.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this a mutex that we use in ggml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ggerganov ggerganov Apr 25, 2025

Choose a reason for hiding this comment

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

Does it make a difference if this becomes:

static std::mutex ggml_critical_section_mutex;

I.e. is this CMake flag still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I.e. is this CMake flag still necessary

Unfortunately, just this change still produces the error:

> git diff
warning: in the working copy of 'bindings/javascript/package.json', CRLF will be replaced by LF the next time Git touches it
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 34ef7958..070818e5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -149,7 +149,7 @@ if (NOT TARGET ggml)
             #
             # Specifically to whisper.cpp this would cause a crash when using the Java bindings.
             # resulting in a Invalid memory access error.
-            target_compile_definitions(ggml-base PRIVATE _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
+            #target_compile_definitions(ggml-base PRIVATE _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
         endif()
     endif()
     # ... otherwise assume ggml is added by a parent CMakeLists.txt
diff --git a/ggml/src/ggml-threading.cpp b/ggml/src/ggml-threading.cpp
index 25a19eed..8f6b2b69 100644
--- a/ggml/src/ggml-threading.cpp
+++ b/ggml/src/ggml-threading.cpp
@@ -1,7 +1,7 @@
 #include "ggml-threading.h"
 #include <mutex>

-std::mutex ggml_critical_section_mutex;
+static std::mutex ggml_critical_section_mutex;

 void ggml_critical_section_start() {
     ggml_critical_section_mutex.lock();
whisper_init_from_file_with_params_no_state: loading model from '../../models/ggml-tiny.en.bin'
whisper_init_with_params_no_state: use gpu    = 1
whisper_init_with_params_no_state: flash attn = 0
whisper_init_with_params_no_state: gpu_device = 0
whisper_init_with_params_no_state: dtw        = 0

WhisperCppTest > initializationError FAILED
    java.lang.Error: Invalid memory access
        at com.sun.jna.Native.invokePointer(Native Method)
        at com.sun.jna.Function.invokePointer(Function.java:497)
        at com.sun.jna.Function.invoke(Function.java:441)
        at com.sun.jna.Function.invoke(Function.java:361)
        at com.sun.jna.Library$Handler.invoke(Library.java:270)
        at jdk.proxy3/jdk.proxy3.$Proxy12.whisper_init_from_file_with_params(Unknown Source)
        at io.github.ggerganov.whispercpp.WhisperCpp.initContextImpl(WhisperCpp.java:63)
        at io.github.ggerganov.whispercpp.WhisperCpp.initContext(WhisperCpp.java:39)
        at io.github.ggerganov.whispercpp.WhisperCppTest.init(WhisperCppTest.java:28)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the detailed investigation. @slaren FYI

@danbev danbev requested a review from ggerganov April 25, 2025 08:02
@danbev danbev merged commit 1c20f46 into ggml-org:master Apr 25, 2025
51 checks passed
@danbev danbev deleted the ci-enable-bindings-java-job branch April 28, 2025 11:17
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