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

vulkan: fix build for GGML_VULKAN_RUN_TESTS, add TFLOPS to logging #961

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

jeffbolznv
Copy link
Contributor

GGML_VULKAN_RUN_TESTS currently has build errors like:

ggml-vulkan.cpp(5251,5): error C2664: 'void ggml_vk_dispatch_pipeline(ggml_backend_vk_context *,vk_context &,vk_pipeline &,const std::initializer_list<vk::DescriptorBufferInfo> &,size_t,const void *,std::array<uint32_t,3>)': cannot convert argument 4 from 'initializer list' to 'const std::initializer_list<vk::DescriptorBufferInfo> &'
ggml-vulkan.cpp(5497,10): error C2039: 'staging': is not a member of 'ggml_backend_vk_context'

and runtime assertions like:

ggml-vulkan.cpp:2486: GGML_ASSERT(pipeline->descriptor_set_idx < pipeline->descriptor_sets.size()) failed

Fix these, and also add teraflops calculation to the matmul tests.

@jeffbolznv
Copy link
Contributor Author

Ping for review.

I'm curious, is it better to do these sort of fixes in llama.cpp or in ggml?

@ggerganov
Copy link
Owner

Sorry for the delay. Pinging @0cc4m as they are the main maintainer of the vulkan backend.
If we don't get a response in a day or two, will try to review myself - please ping again if necessary.

I'm curious, is it better to do these sort of fixes in llama.cpp or in ggml?

llama.cpp has more eyes in general, so chances for review are better. Although some of the Vulkan PRs are also stale over there (ggerganov/llama.cpp#9407).

Copy link
Collaborator

@0cc4m 0cc4m left a comment

Choose a reason for hiding this comment

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

Apart from that copyright note, I think this is fine.

@@ -1,3 +1,6 @@
// SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: MIT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this note? I don't think this is appropriate. If we put a copyright note here, it'd have to mention others before Nvidia.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's company policy to add a copyright notice in files that are modified. I'm happy to add other notes here as well, maybe Copyright (c) 2023-2024 The ggml authors like is in some other files?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm the main author of this file, but many people have contributed. Should we mention them all? The more significant ones? Or just a group?

@ggerganov Do you have an opinion here?

Copy link
Owner

Choose a reason for hiding this comment

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

@ggerganov Do you have an opinion here?

I don't really understand the necessity for adding these SPDX identifiers in the source files. The copyright and license of the project is already specified in the LICENSE file, so these identifiers are IMO pointless.

Should we mention them all? The more significant ones? Or just a group?

We definitely don't want to maintain redundant copyright notices and license identifiers in the source files. There is commit history already available which provides all that information in greater detail. Any entity that wants to claim copyright should be added to the AUTHORS list - this is the main purpose for the file to exist and it's much easier to maintain.

IMO the simplest solution is to remove the identifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I think the AUTHORS file will suffice. I've regenerated that file and removed the copyright comments.

@ggerganov ggerganov merged commit 3a423e6 into ggerganov:master Sep 27, 2024
4 checks passed
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