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

Proposal: Improve llama.cpp snippet #778

Merged
merged 8 commits into from
Aug 7, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 44 additions & 22 deletions packages/tasks/src/local-apps.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import type { ModelData } from "./model-data";
import type { PipelineType } from "./pipelines";

type Snippet = {
title: string;
setup: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setup: string;
setup?: string;

make the setup step optional. Maybe some snippets will not need the setup step

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7983478

command: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
command: string;
content: string;

rename command -> content to be consistent with how we name this kind of stuff in hf.co codebase

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions @mishig25 . Sorry for the late response, I'll have a look later this week when I have more time !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7983478

};

/**
* Elements configurable by a local app.
*/
Expand Down Expand Up @@ -39,36 +45,52 @@ export type LocalApp = {
* And if not (mostly llama.cpp), snippet to copy/paste in your terminal
* Support the placeholder {{GGUF_FILE}} that will be replaced by the gguf file path or the list of available files.
*/
snippet: (model: ModelData) => string | string[];
snippet: (model: ModelData) => Snippet | Snippet[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
snippet: (model: ModelData) => Snippet | Snippet[];
snippet: (model: ModelData) => string | string[] | Snippet | Snippet[];

to be backward compatible (and to compile)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 98a2637

But I would recommend to move completely to Snippet interface, because my idea is to have dedicated "title" to explain what each snippet does.

}
);

function isGgufModel(model: ModelData) {
return model.tags.includes("gguf");
}

const snippetLlamacpp = (model: ModelData): string[] => {
const snippetLlamacpp = (model: ModelData): Snippet[] => {
const command = (binary: string) =>
[
"# Load and run the model:",
`${binary} \\`,
' --hf-repo "${model.id}" \\',
" --hf-file {{GGUF_FILE}} \\",
' -p "You are a helpful assistant" \\',
" --conversation",
Vaibhavs10 marked this conversation as resolved.
Show resolved Hide resolved
].join("\n");
return [
`# Option 1: use llama.cpp with brew
brew install llama.cpp

# Load and run the model
llama \\
--hf-repo "${model.id}" \\
--hf-file {{GGUF_FILE}} \\
-p "I believe the meaning of life is" \\
-n 128`,
`# Option 2: build llama.cpp from source with curl support
git clone https://github.com/ggerganov/llama.cpp.git
cd llama.cpp
LLAMA_CURL=1 make

# Load and run the model
./main \\
--hf-repo "${model.id}" \\
-m {{GGUF_FILE}} \\
-p "I believe the meaning of life is" \\
-n 128`,
{
title: "Install from brew",
setup: "brew install llama.cpp",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw unrelated but wondering is llama.cpp is on winget? cc @mfuntowicz too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked here: https://winget.run and didn't find any. To think of it, it'd be a pretty cool idea to add that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it would be nice to have, knowing that llama.cpp already have pre-built binary via CI. Unfortunately I'm not very familiar with windows stuff, so I'll create an issue on llama.cpp to see if someone can help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created the issue: ggerganov/llama.cpp#8188

command: command("llama-cli"),
},
{
title: "Use pre-built binary",
setup: [
// prettier-ignore
"# Download pre-built binary from:",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually LGTM. Just wondering if this doesn't bloat the overall UI for snippets for a user i.e. we present too many options to the end-user.

(just food for thought)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I switched my mind to "UX/UI designer" when I drafted this proposal ;-)

The current UI has a problem that these multiple snippets but no title for them (visually hard to distinct between the 2 snippets):

image

My first iteration would be to have a title for each snippet, then only "expand" one section at a time (while other options are "collapsed")

image

But then I think we can also split between the "setup" and "run" step, since ideally the user will setup just once but run multiple times.

Feel free to give other suggestions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea I quite like the second image. Let's ask @gary149/ @julien-c for thoughts here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @gary149 wdyt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bumping this as I think it'll be good to merge this soon! cc: @gary149 (sorry for creating an extra notification)

Copy link
Collaborator

@gary149 gary149 Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay on the UI, but let's see if it's not busted on the Hub side. (sorry for the late reply)

"# https://github.com/ggerganov/llama.cpp/releases",
].join("\n"),
command: command("./llama-cli"),
},
{
title: "Build from source code",
setup: [
"# Install required packages",
"sudo apt install build-essential libcurl4-openssl-dev",
Vaibhavs10 marked this conversation as resolved.
Show resolved Hide resolved
"",
"# Clone and build",
"git clone https://github.com/ggerganov/llama.cpp.git",
"cd llama.cpp",
"LLAMA_CURL=1 make -j llama-cli",
].join("\n"),
command: command("./llama-cli"),
},
];
};

Expand Down
Loading