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

Added server example themes support with two sample themes and a favicon. #6848

Merged
merged 17 commits into from
May 8, 2024

Conversation

jboero
Copy link
Contributor

@jboero jboero commented Apr 23, 2024

I've always thought the --path flag could be used to support basic UI themes and also thought it should have a favicon. I've added these and tested them out. Have a look.

@jboero
Copy link
Contributor Author

jboero commented Apr 25, 2024

Fixed whitespace

@jboero
Copy link
Contributor Author

jboero commented Apr 25, 2024

image

Check actions cancelled for some other priority job and I can't seem to manually re-run them, so MOAR OPACITY
Trying to re-trigger the cancelled action.
This Actions pipeline is failing for random issues.
@jboero
Copy link
Contributor Author

jboero commented Apr 27, 2024

Ugh an issue with the build environment seems to be killing these Actions.

ModuleNotFoundError: No module named 'distutils'
https://github.com/ggerganov/llama.cpp/actions/runs/8859670654/job/24329697021#step:7:116

@jboero
Copy link
Contributor Author

jboero commented Apr 27, 2024

I think an update to our Actions environment is actually blocking these. Anybody else hitting this error?
#6422

@jboero
Copy link
Contributor Author

jboero commented Apr 29, 2024

@ggerganov did something change in testing automation? This PR is purely additive and shouldn't break anything else but automation keeps tripping on the same error as this python 3.12 support issue.

#6422

@ggerganov
Copy link
Owner

I think you need to rebase on latest master - there were some changes to the CI that are missing in this PR as it is

Btw, it seems the themes are copy-pasting the .js/.html code. Can this be avoided?
Otherwise, making a change to the frontend would have to be replicated across all themes - does not sound OK

@jboero
Copy link
Contributor Author

jboero commented Apr 29, 2024

Good point I'll try rebasing.

Otherwise, making a change to the frontend would have to be replicated across all themes - does not sound OK

I thought about symlinks for unchanged files but then there is the opposite problem. Changing one theme affects and/or breaks the other themes. I thought these were copied examples. What would you propose? This is simple just using the --path arg.

@jboero
Copy link
Contributor Author

jboero commented May 7, 2024

@ggerganov any bump on this? Since the UI is effectively rendered completely using the contents of the public folder it makes sense to me to change copies for themes and just use --path ./theme_dir

@ggerganov
Copy link
Owner

Let's give this a try, but first remove the following files from the themes:

  • index.js
  • completion.js
  • json-schema-to-grammar.mjs

Since these are unchanged, they will be served from the static contents embedded in the server app:

// using embedded static files
svr->Get("/", handle_static_file(index_html, index_html_len, "text/html; charset=utf-8"));
svr->Get("/index.js", handle_static_file(index_js, index_js_len, "text/javascript; charset=utf-8"));
svr->Get("/completion.js", handle_static_file(completion_js, completion_js_len, "text/javascript; charset=utf-8"));
svr->Get("/json-schema-to-grammar.mjs", handle_static_file(
json_schema_to_grammar_mjs, json_schema_to_grammar_mjs_len, "text/javascript; charset=utf-8"));

Also rename all files that have underscore to use a dash for consistency

@jboero
Copy link
Contributor Author

jboero commented May 7, 2024

I see. I thought the path dir was all or nothing but yes that makes sense. OK let me clean it up a bit.

jboero added 7 commits May 7, 2024 14:14
This will be served from the static string built-in to server.
This will be served from the static string built-in to server.
This will be served from the static string built-in to server.
This will be served from the static string built-in to server.
This will be served from the static string built-in to server.
This will be served from the static string built-in to server.
@jboero
Copy link
Contributor Author

jboero commented May 7, 2024

Sloppy commits but done. Thanks @ggerganov

@ggerganov ggerganov merged commit bd1871f into ggerganov:master May 8, 2024
24 checks passed
@scottstirling
Copy link

How are folks gonna customize browser presentation code buried in the C++ binary? Or is the point to hinder that?

Every tweak to the UI will require a make to compile reload and test, defeating the utility & practicality of externalized user interface code.

@jboero
Copy link
Contributor Author

jboero commented May 9, 2024

Every tweak to the UI will require a make to compile reload and test, defeating the utility & practicality of externalized user interface code.

By separating the UI elements (--path [UIDIR]) and having the UI simply map to the API I don't think there is much to worry about. The static strings that become defaults in the C++ can simply be overridden by placing your file in the --path directory at runtime. Simple but effective actually or am I misunderstanding your predicament? This is how the binary has been built for quite a while.

@jboero jboero deleted the theme branch May 10, 2024 19:17
@scottstirling
Copy link

Thanks for the clarification. I will pull the change and give it a try.

@mofosyne mofosyne added Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level server/webui examples server labels May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level server/webui server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants