-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Restore clip's cb() to its rightful glory - extract common debugging elements in llama #17914
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to go a step ahead, I would suggest using ggml_backend_sched_set_eval_callback to make it works the same way as libllama. This will be a cleaner solution
tools/mtmd/clip.cpp
Outdated
| std::string t_name = std::string(name) + "_" + std::to_string(il); | ||
| ggml_tensor * args[] = { t }; | ||
| ggml_tensor * res = ggml_custom_4d(ctx0, t->type, t->ne[0], t->ne[1], t->ne[2], t->ne[3], args, 1, print_debug, 1, nullptr); | ||
| strcpy(res->name, t_name.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ggml_set_name instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better, ggml_format_name
tools/mtmd/clip.cpp
Outdated
| ggml_tensor * args[] = { t }; | ||
| ggml_tensor * res = ggml_custom_4d(ctx0, t->type, t->ne[0], t->ne[1], t->ne[2], t->ne[3], args, 1, print_debug, 1, nullptr); | ||
| strcpy(res->name, t_name.c_str()); | ||
| ggml_build_forward_expand(gf, res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should guard the whole thing under ctx->debug_graph. seems like it's got removed by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah :>
tools/mtmd/clip.cpp
Outdated
| #include "ggml-cpp.h" | ||
| #include "ggml-alloc.h" | ||
| #include "ggml-backend.h" | ||
| #include "ggml/src/ggml-impl.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed - we cannot include internal header from ggml
tools/mtmd/clip.cpp
Outdated
| #include <cstring> | ||
| #include <fstream> | ||
| #include <map> | ||
| #include <memory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of these are already included by clip-impl.h - do we really need to include them again here?
…D_DEBUG_GRAPH with same functionality
|
All right, based on the convo with @ngxson I've decided to tackle this properly:
|
|
I would very much like to extend the callback procedure to (a) make it also possible in other clients (such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be inside common/debug.h instead. There is no internal libllama components using these functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, fair point.
I used my callback function from my Qwen3Next testing days, it seems like it works more cleanly than the previous one which was causing some problems with the scheduler / buffers.