Include server in releases + other build system cleanups

[KerfuffleV2]

Set LLAMA_BUILD_SERVER in workflow so the server example gets build, however I only added this to the Windows builds because it seems like only Windows binary artifacts are included in releases.

Add server example to Makefile (still uses LLAMA_BUILD_SERVER define and does not build by default)

Remove vdot binary when running make clean.

Fix compile warnings in server example.

Add .hpp files to workflow (the server example has one).

---

I don't really have a way to test the workflow changes. They seem pretty straightforward but I'm far from an expert on GitHub workflows.

I also don't actually know C++, so my changes to server.cpp were done without really understanding how it works. All I can say is it appears to run okay and compiles without warnings on clang 15, gcc 13 and gcc 11.

Closes #1578

[KerfuffleV2]

-DLLAMA_BUILD_SERVER=ON should probably also be added to cmake_command in tidy-review.yml so changes involving the server example are subject to linting?

[Green-Sky]

afaik @SlyEcho was doing something with the server binaries too?

[KerfuffleV2]

afaik @SlyEcho was doing something with the server binaries too?

I looked at the pending pulls and didn't see anything obvious that overlapped but I might have missed it.

[Green-Sky]

we should merge #1606 this before, to fix ci

can you take a very quick look and merge? :)

otherwise looks good

[KerfuffleV2]

@Green-Sky

we should merge #1606 this before, to fix ci
can you take a very quick look and merge? :)

It looks like a simple change but I don't have a way to test it (I'm not on Windows). Do you still want me to just go ahead and merge it? (Or you can?)

[Green-Sky]

yea i just merged it. pull and let ci run :)

[KerfuffleV2]

@Green-Sky

Looks like the Windows clBLAST stuff doesn't work: https://github.com/ggerganov/llama.cpp/actions/runs/5098706551

What do you want to do at this point?

[Green-Sky]

this is very weird. let's wait for @SlyEcho to fix it :)

[Green-Sky]

also the cublas build failing is gg's doing 😄

[SlyEcho]

I didn't manage to make a PR before hitting the other issues, but you can compare what I did here: master...SlyEcho:llama.cpp:enable_server

I also added it to the Docker image and command.

[KerfuffleV2]

@SlyEcho

you can compare what I did here:

I might be biased, but I like my approach to the Makefile changes better than yours. I take the .hpp file into account, use the same environment variable as with cmake to control building and also make clean will clean up that binary.

If you applied your pull after mine, you could just remove the Makefile changes and do RUN make LLAMA_BUILD_SERVER=1 to build the server. The rest of it doesn't look like it has conflicts with this pull.

[SlyEcho]

I didn't pay much attention to the Makefile but I have some justification: the HTTP and JSON single file libraries are external, so it is not expected that they change at all between builds, but I guess this assumption is not technically correct.

I think the json.hpp file problem has an easier solution: just rename it to json.h?

[Green-Sky]

I think the json.hpp file problem has an easier solution: just rename it to json.h?

I'm against renaming dependencies. Also, technically, C and C++ headers can be different and I prefer using .hpp for c++ headers everywhere.

[KerfuffleV2]

the HTTP and JSON single file libraries are external, so it is not expected that they change at all between builds

I doubt they'll change frequently, but it would be weird and confusing if someone copied in a newer version of those libraries and running make didn't actually rebuild server with them. Presumably they'll be upgraded periodically.

[SlyEcho]

I don't mean to suggest to rewrite everything but there is actually a better solution to this:

server: examples/server/server.cpp examples/server/httplib.h examples/server/json.hpp build-info.h ggml.o llama.o common.o $(OBJS)
	$(CXX) $(CXXFLAGS) -Iexamples/server $(filter-out %.h,$(filter-out %.hpp,$^)) -o $@ $(LDFLAGS)

to:

%.o: %.cpp
    $(CXX) $(CPPFLAGS) $(CXXFLAGS)  -c -o $@ $<
%: %.o
    $(CXX) $(LDFLAGS) -o $@ $^ $(LDLIBS)
VPATH+= examples/server
server.o: build-info.h httplib.h json.hpp

something like that

[KerfuffleV2]

I don't mean to suggest to rewrite everything but there is actually a better solution

I agree that approach is better, but I wasn't really looking to refactor the Makefile with this pull. Cleaning up the compile warnings in server.cpp was kind of scope creep, but I figured I had to do that to prevent checks from failing.

I also am not really sure about making changes to the Makefile which will work on all supported platforms, various versions and flavors of make, etc. That's why I just cut-and-pasted from one of the other examples which was presumably correct.

Do you feel strongly that refactoring the Makefile needs to happen in this pull? Is there anything else that has to change before it is mergeable (aside from fixing the current conflicts of course)?

[SlyEcho]

Do you feel strongly that refactoring the Makefile needs to happen in this pull?

Absolutely not.

[digiwombat]

I'd be fine if this rolls in any of the changes from my PR (#1570) as well, and I can close mine.

Currently the server assumes a way of handling the context that doesn't really fit with most frontends, which do heavy context editing. It's also missing a number of generation settings that could just be passed directly though the server but aren't implemented.

Welp, merged while I was typing this so nevermind. Haha.