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

Only show -ngl option when relevant + other doc/arg handling updates #1625

Merged

Conversation

KerfuffleV2
Copy link
Collaborator

This pull:

  1. Adds a LLAMA_SUPPORTS_GPU_OFFLOAD define to llama.h (defined when compiled with clBLAST or cuBLAST)
  2. Updates the argument handling in the common example code to only show the -ngl, --n-gpu-layers option when GPU offload is possible.
  3. Adds an entry for the -ngl, --n-gpu-layers option to the main and server examples documentation
  4. Updates main and server examples documentation to use the new style dash separator argument format
  5. Updates the server example to use dash separators for its arguments and adds -ngl to --help (only shown when compiled with appropriate support). It will still support --memory_f32 and --ctx_size for compatibility.
  6. Adds a warning discouraging use for --memory-f32 for the main and server examples --help text as well as documentation. Rationale: Is there any perplexity data for using 16bit vs 32bit memory? #1593 (reply in thread)

@JohannesGaessler Hopefully this isn't stepping on your toes, I took a different approach to dealing with the GPU offload support issue.

Closes #1555

@JohannesGaessler
Copy link
Collaborator

In terms of usability I think any binaries should give you an error message telling you that you need compile option XY to use some feature. An error message that just tells you that some argument doesn't exist is I think much less helpful for users that are unaware of the compilation option even existing.

@KerfuffleV2
Copy link
Collaborator Author

KerfuffleV2 commented May 28, 2023

An error message that just tells you that some argument doesn't exist is I think much less helpful for users that are unaware of the compilation option even existing.

You're just talking about display a better error if the user specifies the option that wasn't compiled in, not saying a bunch of detailed information about how to enable it should be added to the --help text. Correct?


edit: How about this? Last commit changes the behavior when using -ngl or --n-gpu-layers when support isn't compiled in: It now just shows a warning rather than erroring out and advises the user to check the README for instructions on how to compile with GPU BLAS support.

Refer to CLBlast and cuBLAS correctly in doc changes.
@JohannesGaessler
Copy link
Collaborator

You're just talking about display a better error if the user specifies the option that wasn't compiled in, not saying a bunch of detailed information about how to enable it should be added to the --help text. Correct?

Yes. It's enough to e.g. tell the user that the feature doesn't work because llama.cpp wasn't compiled with cuBLAS/clblast. The user just needs to be told that something is wrong so they'll know to look it up.

How about this? Last commit changes the behavior when using -ngl or --n-gpu-layers when support isn't compiled in: It now just shows a warning rather than erroring out and advises the user to check the README for instructions on how to compile with GPU BLAS support.

I would prefer an error over a warning since people have a tendency not to read warnings but a warning would be fine too - the program just shouldn't silently ignore the CLI argument.

@KerfuffleV2
Copy link
Collaborator Author

I would prefer an error over a warning since people have a tendency not to read warnings but a warning would be fine too

I can change it if you feel strongly. Otherwise, are you okay with the current behavior? The warning is two lines and the result of ignoring it is also pretty benign: possibly lower performance.

@JohannesGaessler
Copy link
Collaborator

I don't feel strongly about it. As I said: a warning would be fine too.

@KerfuffleV2 KerfuffleV2 merged commit 1b78ed2 into ggerganov:master May 28, 2023
@KerfuffleV2 KerfuffleV2 deleted the feat-ngl_arg_only_when_supported branch September 6, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ISSUE] --gpu-layers in useless.
3 participants