-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
WIP Feature/cpp #4155
WIP Feature/cpp #4155
Conversation
2a470fa
to
f62b81d
Compare
Hm, I don't think ggerganov would approve of this. https://github.com/ggerganov/ggml is written in C for simplicity. |
Yes, I understand that, but lets have a conversation about it?
…On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***> wrote:
Hm, I don't think ggerganov would approve of this.
https://github.com/ggerganov/ggml is written in C for simplicity.
—
Reply to this email directly, view it on GitHub
<#4155 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I think this could uncover some bugs and I might have found some invalid
type conversions already. We should plan to maintain a C++ branch of the
code in parallel for a few months.
…On Tue, Nov 21, 2023, 12:11 Jim Dupont ***@***.***> wrote:
Yes, I understand that, but lets have a conversation about it?
On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***>
wrote:
> Hm, I don't think ggerganov would approve of this.
> https://github.com/ggerganov/ggml is written in C for simplicity.
>
> —
> Reply to this email directly, view it on GitHub
> <#4155 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
for example converting a float to an int might not be what you want
```ggml.cpp:5494:46: warning: narrowing conversion of ‘p0’ from ‘float’ to
‘int32_t’ {aka ‘i\nt’} [-Wnarrowing]
5494 | int32_t params[] = { op, k0, k1, s0, s1, p0, p1 };
| ^~
```
…On Tue, Nov 21, 2023, 12:23 Jim Dupont ***@***.***> wrote:
I think this could uncover some bugs and I might have found some invalid
type conversions already. We should plan to maintain a C++ branch of the
code in parallel for a few months.
On Tue, Nov 21, 2023, 12:11 Jim Dupont ***@***.***> wrote:
> Yes, I understand that, but lets have a conversation about it?
>
> On Tue, Nov 21, 2023, 12:10 Jared Van Bortel ***@***.***>
> wrote:
>
>> Hm, I don't think ggerganov would approve of this.
>> https://github.com/ggerganov/ggml is written in C for simplicity.
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#4155 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AD5KQ2N4NGKHQ6PZZY2HDXTYFTOABAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGMZDIOBVG4>
>> .
>> You are receiving this because you authored the thread.Message ID:
>> ***@***.***>
>>
>
|
You can also build master with |
yes but that is not the point here, the point is that getting it to compile
on c++ can uncover things and highlight things and is a worthwhile
exercize. Also the usage of templates could clean up the code a lot.
…On Tue, Nov 21, 2023, 12:55 Jared Van Bortel ***@***.***> wrote:
for example converting a float to an int might not be what you want
You can also build master with cmake -B build
-DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this
warning. That probably is a bug, so perhaps we should enable
-Wfloat-conversion by default.
—
Reply to this email directly, view it on GitHub
<#4155 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5KQ2JIQG3ZGTJ3XKXN3WLYFTTIDAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGM4TIOJSGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
now compiling again,
19a8830
to
ee9b0bc
Compare
This is another thing that popped up, bunch of the types dont have the full complement of functions
|
@@ -718,7 +718,7 @@ add_library(llama | |||
) | |||
|
|||
target_include_directories(llama PUBLIC .) | |||
target_compile_features(llama PUBLIC cxx_std_11) # don't bump | |||
target_compile_features(llama PUBLIC cxx_std_20) # don't bump |
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.
Yeah, that's probably not gonna happen either... for the most part this project uses a subset of C++11 that barely even includes templates. Kerfuffle and I are proposing to add some in #4092, we'll see how it goes - it could be that it ends up getting rewritten to do things the C way. C++20, a language which has modules, coroutines, and concepts, is almost certainly out of the question.
The future is going to happen in the the future for sure, I am super
excited I got this done in one day. It does not matter if you merge it, it
can live as a compatible branch for now.
…On Tue, Nov 21, 2023, 21:23 Jared Van Bortel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CMakeLists.txt
<#4155 (comment)>:
> @@ -718,7 +718,7 @@ add_library(llama
)
target_include_directories(llama PUBLIC .)
-target_compile_features(llama PUBLIC cxx_std_11) # don't bump
+target_compile_features(llama PUBLIC cxx_std_20) # don't bump
Yeah, that's probably not gonna happen either... for the most part this
project uses a subset of C++11 that barely even includes templates.
Kerfuffle and I are proposing to add some in #4092
<#4092>, we'll see how it goes
- it could be that it ends up getting rewritten to do things the C way.
C++20, a language which has modules, coroutines, and concepts, is almost
certainly out of the question.
—
Reply to this email directly, view it on GitHub
<#4155 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5KQ2IKZ73NPA5GU73FY3TYFVOZBAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBTGQ2DKNJSGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
It's not a bug - it was added with the YOLO example to support padding with odd number: It needs better comment for Regarding turning We can think about adding C++ bindings as proposed in ggerganov/ggml#581 |
Well one of the things that I'm looking into is that we can generate better
bindings using the template metaprogramming and then we can use that inside
of the C code so I'm thinking we can generate code to be used in the Legacy
version
…On Wed, Nov 22, 2023, 08:09 Georgi Gerganov ***@***.***> wrote:
for example converting a float to an int might not be what you want
You can also build master with cmake -B build
-DCMAKE_C_FLAGS='-Wfloat-conversion' && make -C build main to see this
warning. That probably is a bug, so perhaps we should enable
-Wfloat-conversion by default.
It's not a bug - it was added with the YOLO example to support padding
with odd number:
ggerganov/ggml#576 <ggerganov/ggml#576>
It needs better comment for ggml_pool_2d and explicit cast to int
Regarding turning ggml to C++ - definitely not an option.
But appreciate the effort in this PR and don't mind having it live as a
separate branch.
We can think about adding C++ bindings as proposed in ggerganov/ggml#581
<ggerganov/ggml#581>
—
Reply to this email directly, view it on GitHub
<#4155 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5KQ2JJE6DPFZMLWZLFEQDYFX2PBAVCNFSM6AAAAAA7U2PIF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSG42DCOBWGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Also If we can just refactor those bits like the initializers so the live in a different module or use conditional compliation then we can have the code compile without change in c++and c as well. I am not trying to refactor everything or remove malloc etc. So this could actually be cleaned up so it would be an optional flag. |
#4209 see this better pr |
This is a work in progress to compile with C++ and add in the magic header,
I just got the first version to compile by commenting out the initializers and things that did not work.
am sharing this with the community for comments, please do not merge.