ggerganov / llama.cpp

LLM inference in C/C++
MIT License
67.81k stars 9.73k forks source link

Clean up server code #5762

Open ngxson opened 8 months ago

ngxson commented 8 months ago

Motivation

As seen on https://github.com/ggerganov/llama.cpp/issues/4216 , one of the important task is to refactor / clean up the server code so that it's easier to maintain. However, without a detailed plan, personally I feel like it's unlikely to be archived.

This issue is created so that we can discuss about how to refactor or clean up the code.

The goal is to help existing and new contributors to easily find out where to work in the code base.

Current architecture

The current server implementation has 2 thread: one for HTTP part and one for inference.

image

Ideas

Feel free to suggest any ideas that you find helpful (please keep in mind that we do not introduce new features here, just to re-write the code):

ngxson commented 8 months ago

CC @phymbert and @ggerganov

Do you have any idea for the html / js stuff? Personally I never touched this part before, because I have my own frontend implementation on my side using vitejs.

ggerganov commented 8 months ago

No more hard-coding js files into hpp, as these files pollute the code base. They should be converted to hpp by using code generation

This is not needed. The reason to embed them as byte arrays in .hpp files is to have the UI embedded in the application. Otherwise, the app would need to read external files

Our html / js code is not that big, is it worth to split them into separated html / js files? In fact, html + css + js can be in one single file, but should we do that in our case?

Do you have any idea for the html / js stuff? Personally I never touched this part before, because I have my own frontend implementation on my side using vitejs.

The UI code is fine - I wouldn't look to change it for now

I would probably start with:

void update_system_prompt();
void notify_system_prompt_changed();
void process_system_prompt_data();

These would be better named as:

void system_prompt_update();
void system_prompt_notify();
void system_prompt_process();

Similar thoughts about variable names. For example, we have:

But then also:

It might seem like unimportant or opinionated change, but IMO having consistency in the names significantly improves the code quality and makes it easier to understand the relations between things

Overall, just try to establish some patterns and consistency. No need for abstractions

ngxson commented 8 months ago

This is not needed. The reason to embed them as byte arrays in .hpp files is to have the UI embedded in the application. Otherwise, the app would need to read external files

I mean these .hpp file instead of being inside the repo, they can be generated from the source file (html / js) in build time, much like the way we generate build-info.cpp

The UI code is fine - I wouldn't look to change it for now

Thanks for the confirmation, I'll leave them untouched.

Your suggestions regarding renaming are clear for me and I'll having a look into this in the next days, I also agree that there is no need for more abstractions at the moment.

The utils.hpp does not have much inside it because initially I don't want the move to introduce too many breaking changes to other opening PR. But with your confirmation I'm more confident now to start moving struct to utils.hpp progressively.