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(go binding): add beamsize/entropythold/maxcontext to context interface #2350

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

hsinhoyeh
Copy link
Contributor

fixes: #2349

@hsinatfootprintai
Copy link

@ggerganov I think the build failed for go building has been fixed. please check it.

@hsinhoyeh
Copy link
Contributor Author

related issue: #2330 #2339

@hsinatfootprintai
Copy link

@ggerganov seems it didn't re-test automatically, require manually trigger

@ggerganov
Copy link
Owner

Just triggered it again - let's see if it's green

@hsinhoyeh
Copy link
Contributor Author

@ggerganov it is green now. it seems that we need a static library libwhisper.a as well as a dynamic library libggml.so.
is it intentional? making a clean path for lamma.cpp I guess?

@ggerganov
Copy link
Owner

it seems that we need a static library libwhisper.a as well as a dynamic library libggml.so.

In what sense we need both?

@hsinhoyeh
Copy link
Contributor Author

it seems that we need a static library libwhisper.a as well as a dynamic library libggml.so.

In what sense we need both?

okey, so it is clear that this is NOT intentional...

@hsinhoyeh
Copy link
Contributor Author

it seems that we need a static library libwhisper.a as well as a dynamic library libggml.so.

In what sense we need both?

@ggerganov this CL makes much more sense now.

Makefile Outdated
@@ -967,11 +967,14 @@ src/whisper.o: \

$(LIB_WHISPER): \
$(OBJ_WHISPER) \
$(OBJ_GGML) \
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it is necessary to include the ggml object files in the whisper dynamic lib. We want to link ggml dynamically

Choose a reason for hiding this comment

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

ya, removed.

@hsinhoyeh
Copy link
Contributor Author

@ggerganov This issue may require your attention, as similar issues with Go bindings keep recurring.

Makefile Outdated
Comment on lines 974 to 976
$(OBJ_WHISPER) \
$(OBJ_GGML) \
$(LIB_GGML)
Copy link
Owner

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Contributor Author

@hsinhoyeh hsinhoyeh Aug 28, 2024

Choose a reason for hiding this comment

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

$(LIB_GGML) is not necessary, but $(OBJ_GGML) is required.

@ggerganov ggerganov merged commit c4e1861 into ggerganov:master Aug 28, 2024
47 checks passed
bygreencn added a commit to bygreencn/whisper.cpp that referenced this pull request Sep 3, 2024
* ggerganov/master: (40 commits)
  revert : cmake : set MSVC to use UTF-8 on source files (ggerganov#2346)
  sync : ggml
  ggml: fix ggml_graph_cpy undefined behavior (ggml/943)
  cann : fix doxy (ggml/0)
  vulkan : fix build (llama/0)
  cuda : mark BF16 CONT as unsupported
  ggml : fix cont with transposed tensors when one dimension is 1 (ggml/934)
  cmake : set MSVC to use UTF-8 on source files (ggerganov#2346)
  readme : remove invalid flag from Python example (ggerganov#2396)
  readme : fix link (ggerganov#2394)
  go : add beamsize/entropythold/maxcontext to context interface (ggerganov#2350)
  talk-llama : sync llama.cpp
  whisper : update FA call
  sync : ggml
  sync : vulkan (skip) (llama/0)
  ggml : do not crash when quantizing q4_x_x with an imatrix (llama/9192)
  metal : separate scale and mask from QKT in FA kernel (llama/9189)
  ggml : add SSM Metal kernels (llama/8546)
  metal : gemma2 flash attention support (llama/9159)
  CPU/CUDA: Gemma 2 FlashAttention support (llama/8542)
  ...
iThalay pushed a commit to iThalay/whisper.cpp that referenced this pull request Sep 23, 2024
…anov#2350)

* feat(go binding): add beamsize/entropythold/maxcontext to context interface

fixes: ggerganov#2349

* fix go building build

* fix dynamic link .so and header.h

* remove LD_LIBRARY_PATH

* remove ggml obj from whisper dynamic lib

* drop LIB_GGML
iThalay pushed a commit to iThalay/whisper.cpp that referenced this pull request Sep 23, 2024
…anov#2350)

* feat(go binding): add beamsize/entropythold/maxcontext to context interface

fixes: ggerganov#2349

* fix go building build

* fix dynamic link .so and header.h

* remove LD_LIBRARY_PATH

* remove ggml obj from whisper dynamic lib

* drop LIB_GGML
lyapple2008 pushed a commit to lyapple2008/whisper.cpp.mars that referenced this pull request Nov 2, 2024
…anov#2350)

* feat(go binding): add beamsize/entropythold/maxcontext to context interface

fixes: ggerganov#2349

* fix go building build

* fix dynamic link .so and header.h

* remove LD_LIBRARY_PATH

* remove ggml obj from whisper dynamic lib

* drop LIB_GGML
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.

[gobinding] add more methods to context interface
3 participants