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 6 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
62 changes: 40 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";

interface Snippet {
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
interface Snippet {
export interface LocalAppSnippet {

suggestion to export it and name it as LocalAppSnippet so that we can use it from 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.

Fixed in 7983478

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I like the visual improvements mentioned above and we could use them for other snippets as well (i.e., use in transformers), not just local apps. We can discuss about a Snippet type in a separate PR and move forward with this one for now.

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,48 @@ 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, filepath?: string) => string | string[];
snippet: (model: ModelData, filepath?: string) => string | string[] | Snippet | Snippet[];
}
);

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

const snippetLlamacpp = (model: ModelData, filepath?: string): string[] => {
const snippetLlamacpp = (model: ModelData, filepath?: string): Snippet[] => {
const command = (binary: string) =>
[
"# Load and run the model:",
`${binary} \\`,
` --hf-repo "${model.id}" \\`,
` --hf-file ${filepath ?? "{{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 ${filepath ?? "{{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 ${filepath ?? "{{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: [
"git clone https://github.com/ggerganov/llama.cpp.git",
"cd llama.cpp",
"LLAMA_CURL=1 make llama-cli",
].join("\n"),
command: command("./llama-cli"),
},
];
};

Expand Down
Loading