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

metal : reusing llama.cpp logging #3152

Merged
merged 13 commits into from
Sep 27, 2023
Merged

Conversation

Ricardicus
Copy link
Contributor

I wanted to silence some of the outputs and found out that some
came from llama.cpp and some from metal.m on my mac. I saw the "TODO" and
thought I might chime in here with this. Perhaps we should regard the log level also,
and be able to set verbosity via command line arguments? Let me know what you think.

@cebtenzzre
Copy link
Collaborator

The cmake build is failing on macOS:

[ 91%] Linking CXX executable ../../bin/metal
Undefined symbols for architecture x86_64:
  "_llama_log", referenced from:
      _ggml_metal_init in ggml-metal.m.o
      _ggml_metal_free in ggml-metal.m.o
      _ggml_metal_host_malloc in ggml-metal.m.o
      _ggml_metal_add_buffer in ggml-metal.m.o
      _ggml_metal_set_tensor in ggml-metal.m.o
      _ggml_metal_get_tensor in ggml-metal.m.o
      _ggml_metal_graph_find_concurrency in ggml-metal.m.o
      ...
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [bin/metal] Error 1
make[1]: *** [examples/metal/CMakeFiles/metal.dir/all] Error 2

@Ricardicus
Copy link
Contributor Author

Ok, sorry I missed that. Fixed it

@ggerganov
Copy link
Owner

The change in this PR is not OK because it couples ggml with llama.cpp

I didn't write a detailed explanation in the TODO, but what I meant is to implement a way to pass a log callback and have llama.cpp or other projects be able to provide their callback to the metal backend.

@Ricardicus
Copy link
Contributor Author

Ok! I see. I can try to do that, and decouple the two again.

@Ricardicus
Copy link
Contributor Author

Ricardicus commented Sep 13, 2023

I decoupled it. Introducing a log function setter in ggml-metal that is called from llama.cpp that points to the internal one. I still need to include llama.h to get the enum definition. Is this OK?

@Ricardicus
Copy link
Contributor Author

Ricardicus commented Sep 13, 2023

Fixed the trailing whitespace editor config error.

@Ricardicus
Copy link
Contributor Author

I resolved a conflict that had appeared.

@Ricardicus
Copy link
Contributor Author

I decoupled it more now. Since log level is not really used yet I took the liberty to move that into a definition in ggml.h instead. My reasoning was that llama.cpp is already depending on ggml.h, so in order to synchronize the two with a callback I needed a function signature that could be passed in both metal and llama.cpp translation units. Maybe it would be easier to work with ints and macros, but the idea of an enum also makes sense.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

I decoupled it more now.

Yes - this is the way :) I think it OK now - maybe move the llama_log_callback typedef in ggml.h so we can reuse it:

typedef void (*ggml_log_callback)(enum ggml_log_level level, const char * text, void * user_data);

ggml-metal.m Outdated Show resolved Hide resolved
@Ricardicus
Copy link
Contributor Author

I fixed a typedef for log callbacks in ggml.h now :)

@ggerganov
Copy link
Owner

Will merge this some time next week - don't worry, I won't forget :)

@ggerganov ggerganov merged commit dc68974 into ggerganov:master Sep 27, 2023
29 of 33 checks passed
joelkuiper added a commit to vortext/llama.cpp that referenced this pull request Sep 27, 2023
…example

* 'master' of github.com:ggerganov/llama.cpp:
  convert : remove bug in convert.py permute function (ggerganov#3364)
  make-ggml.py : compatibility with more models and GGUF (ggerganov#3290)
  gguf : fix a few general keys (ggerganov#3341)
  metal : reusing llama.cpp logging (ggerganov#3152)
  build : add ACCELERATE_NEW_LAPACK to fix warning on macOS Sonoma (ggerganov#3342)
  readme : add some recent perplexity and bpw measurements to READMES, link for k-quants (ggerganov#3340)
  cmake : fix build-info.h on MSVC (ggerganov#3309)
  docs: Fix typo CLBlast_DIR var. (ggerganov#3330)
  nix : add cuda, use a symlinked toolkit for cmake (ggerganov#3202)
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.

3 participants