Skip to content

Upgrade init_tensor API to return a ggml_status#11854

Merged
slaren merged 2 commits intoggml-org:masterfrom
WilliamTambellini:init_tensor
Feb 28, 2025
Merged

Upgrade init_tensor API to return a ggml_status#11854
slaren merged 2 commits intoggml-org:masterfrom
WilliamTambellini:init_tensor

Conversation

@WilliamTambellini
Copy link
Copy Markdown
Contributor

@WilliamTambellini WilliamTambellini commented Feb 14, 2025

To prepare for an 'abort-free' ggml, as agreeed with Diego in the ggml repo, upgrade the backend init_tensor APIs to return a ggml_status.

Make sure to read the contributing guidelines before submitting a PR

@github-actions github-actions Bot added testing Everything test related Nvidia GPU Issues specific to Nvidia GPUs Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Feb 14, 2025
@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Feb 14, 2025

@slaren review please. Tks.

slaren
slaren reviewed Feb 17, 2025
View reviewed changes
Comment thread ggml/src/ggml-backend.cpp Outdated
Comment thread ggml/src/ggml-cuda/CMakeLists.txt Outdated
Comment thread ggml/src/ggml-cuda/ggml-cuda.cu Outdated
@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Feb 18, 2025

Tks @slaren
Reready for review.

@WilliamTambellini WilliamTambellini mentioned this pull request Feb 18, 2025
Open
slaren
slaren reviewed Feb 18, 2025
View reviewed changes
Comment thread ggml/src/ggml-backend.cpp Outdated
Comment thread tests/test-backend-ops.cpp Outdated
Comment thread ggml/src/ggml-backend.cpp Outdated
@WilliamTambellini WilliamTambellini force-pushed the init_tensor branch 4 times, most recently from e2486eb to 51a0f6c Compare February 18, 2025 23:48
graehl

This comment was marked as outdated.

Sign in to view

graehl
graehl approved these changes Feb 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@graehl graehl left a comment

Choose a reason for hiding this comment

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

ok, so ggml_backend_*_buffer_init_tensor can only return success for most backends but since it's called through the interface init_tensor pointer they still need to return success. was the plan to eventually make cuda_init_tensor sometimes return an error?

@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Feb 19, 2025

Tks @graehl

so ggml_backend_*_buffer_init_tensor can only return success for most backends but since it's called through the interface init_tensor pointer they still need to return success. was the plan to eventually make cuda_init_tensor sometimes return an error?

Yes but that a another PR in the ggml repo

@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Feb 19, 2025

@slaren reready for review please. Best.

matiaslin
matiaslin approved these changes Feb 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@matiaslin matiaslin left a comment

Choose a reason for hiding this comment

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

Good step forward towards the goal of returning an error instead of crashing.

@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Feb 21, 2025

@ggerganov review please

slaren
slaren reviewed Feb 25, 2025
View reviewed changes
Comment thread ggml/src/ggml-alloc.c Outdated
Comment on lines 959 to 983
Copy link
Copy Markdown
Member

@slaren slaren Feb 25, 2025

Choose a reason for hiding this comment

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

This will leak memory if it fails.

Comment thread ggml/src/ggml-backend.cpp Outdated
Comment thread ggml/src/ggml-cuda/CMakeLists.txt Outdated
Comment thread tests/test-backend-ops.cpp Outdated
@WilliamTambellini
Upgrade init_tensor API to return a ggml_status
aa12f29 
To prepare for an 'abort-free' ggml
(ggml not to abort on OOMs but return a OOM status),
as agreeed with Diego in the ggml repo,
upgrade the init_tensor() and view_init() APIs
to return a ggml_status.
@slaren slaren merged commit 70680c4 into ggml-org:master Feb 28, 2025
@ag2s20150909 ag2s20150909 mentioned this pull request Mar 3, 2025
Merged
@WilliamTambellini
Copy link
Copy Markdown
Contributor Author

WilliamTambellini commented Mar 4, 2025

@ggerganov I now have to retouch my PR in ggml. Could you please trigger a sync of ggml from llamacpp to the ggml repo?

@ggerganov
Copy link
Copy Markdown
Member

ggerganov commented Mar 4, 2025

@WilliamTambellini Should be good now.

arthw pushed a commit to arthw/llama.cpp that referenced this pull request Mar 19, 2025
@WilliamTambellini @slaren @arthw
ggml : upgrade init_tensor API to return a ggml_status (ggml-org#11854)
50463a7 
* Upgrade init_tensor API to return a ggml_status

To prepare for an 'abort-free' ggml
(ggml not to abort on OOMs but return a OOM status),
as agreeed with Diego in the ggml repo,
upgrade the init_tensor() and view_init() APIs
to return a ggml_status.

* misc fixes

---------

Co-authored-by: slaren <slarengh@gmail.com>
Seunghhon pushed a commit to Seunghhon/llama.cpp that referenced this pull request Apr 26, 2026
@WilliamTambellini @slaren
ggml : upgrade init_tensor API to return a ggml_status (ggml-org#11854)
17cb3bc 
* Upgrade init_tensor API to return a ggml_status

To prepare for an 'abort-free' ggml
(ggml not to abort on OOMs but return a OOM status),
as agreeed with Diego in the ggml repo,
upgrade the init_tensor() and view_init() APIs
to return a ggml_status.

* misc fixes

---------

Co-authored-by: slaren <slarengh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggerganov ggerganov ggerganov approved these changes

@slaren slaren slaren approved these changes

@matiaslin matiaslin Awaiting requested review from matiaslin

+1 more reviewer

@graehl graehl graehl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ggml changes relating to the ggml tensor library for machine learning Nvidia GPU Issues specific to Nvidia GPUs SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related Vulkan Issues specific to the Vulkan backend

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@WilliamTambellini @ggerganov @graehl @slaren @matiaslin