CMake / CI additions #497
CMake / CI additions #497
Conversation
anzz1
requested review from
Green-Sky,
etra0,
blackhole89,
ggerganov,
j-f1,
prusnak and
sw
There was a problem hiding this comment.
I think it would be better for the builds to be different tasks, in a job. like Test and Build are different. That way you could see which failed without digging into the log. Also, the log would be chaotic and you would not know here in the log which config is used on first glance.
There was a problem hiding this comment.
I thought that too first but then thought to think ahead that if every combination of every platform and compiler flag is each their own step it will quickly be a mess. If there is a code error you'll probably hit it anyway on the first build and the rare case of failing because of the AVX2/AVX/AVX512 flags you would anyway need to dig through the logs to see what happened.
There was a problem hiding this comment.
hm I am not fully convinced. we should wait on a third opinion.
There was a problem hiding this comment.
I am also probably going to setup a build matrix #468 (comment)
There was a problem hiding this comment.
Tbh I'm not certain about my decision anymore 😄
Let's have another opinion.
Basically it boils down to "Build" , "Test" with all the logs concatenated to one log
versus "Build (AVX2)", "Build (AVX)", "Build (AVX512)" and their Test counterparts with own logs.
Both seems good options tho. I'll change it if "option 2" gets a second vote.
There was a problem hiding this comment.
I am also probably going to setup a build matrix #468 (comment)
I would suggest against it, my experiences with gh ci build matrixes are that they are inflexible, harder to configure and prone to bugs. Unless the amount of platforms and compiler options grows to such an substantial amount that maintaining it via the regular granular way becomes so hard that makes it absolutely necessary, I would avoid it at all costs.
edit:
following ur link and checked the whisper.cpp implementation, use of build matrix in that kind of simple step instance is perfectly fine. i was warning about the bad approach of wrapping the whole ci build script in a build matrix like some projects do and then end up doing all sort of special if-cases inside it.
maybe it would be indeed good to use such matrix in both cases (this and #468) .
There was a problem hiding this comment.
how about now? it looks cleaner tbh, it can be little slower tho if the runners are slow to spawn
until i figure out how to check for the capability in gh actions
|
Looks like sanitizer is catching some memory leaks originating from some STL funcs : https://github.com/ggerganov/llama.cpp/actions/runs/4520886595/jobs/7962284415?pr=497 |
CMake: Add AVX512 option
CI: Add AVX/AVX512 builds (Windows)
(AVX512 tests can only be run when the worker happens to support it, building works anyway)
CI: Fix release tagging
(change @zendesk/action-create-release to @anzz1/action-create-release until upstream PR Added commitish as input zendesk/action-create-release#32 is merged)
CMake: Fix sanitizer linkage ( merged cmake: make sanitizers linking #468 )
CI: Add sanitizer builds (Ubuntu)
Edit: everything should be done now.