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

ggml : remove ggml_cplan + rework ggml_cgraph #9431

Closed
wants to merge 2 commits into from

Conversation

ggerganov
Copy link
Owner

target #9408

  • Merge ggml_cplan into ggml_cgraph
  • Rework the ggml_cgraph API
  • Add comments indicating the CPU-specific ggml_cgraph API

I am thinking this could be a step towards removing the CPU-specific ggml interface and aligned with the idea from ggerganov/ggml#949 (comment)

@ggerganov ggerganov changed the title ggml : hide ggml_object, ggml_cgraph, ggml_hash_set ggml : remove ggml_cplan + rework ggml_cgraph Sep 11, 2024
@github-actions github-actions bot added testing Everything test related examples ggml changes relating to the ggml tensor library for machine learning labels Sep 11, 2024
@slaren
Copy link
Collaborator

slaren commented Sep 11, 2024

I am not sure if I understand the purpose of these changes. Mainly it seems to replace the CPU backend cplan API with another set of functions that are also specific to the CPU backend, but it also moves the data previously stored in ggml_cplan to ggml_cgraph, effectively increasing the coupling between the CPU backend and the common ggml code.

@ggerganov
Copy link
Owner Author

The main goal was to make struct ggml_cplan private, but couldn't find a simple way to avoid the return by value of ggml_graph_plan, so decided to merge it in ggml_cgraph. It does increase the coupling so in this regard it might not be worth to make this change.

@slaren
Copy link
Collaborator

slaren commented Sep 11, 2024

If the goal is to access the CPU backend through ggml-backend, then the CPU backend functions (including ggml_cplan) can be made private, and only the ggml-backend interface to the CPU backend (ggml_backend_cpu_init, ggml_backend_cpu_set_n_threads, etc) would need to be exposed.

@ggerganov
Copy link
Owner Author

Right, that would be the better approach. We can do this change at some point to simplify the API. Not high priority though.

@ggerganov ggerganov closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples ggml changes relating to the ggml tensor library for machine learning testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants