Design for serving models with different backends - #81
Conversation
2066d05 to
76fc63e
Compare
leseb
left a comment
There was a problem hiding this comment.
Thanks for working on this proposal! I envision an ideal scenario where we can determine the appropriate backend solely based on the model input (directory or file). I'm questioning whether this is something users really need to worry about. If we can alleviate this hassle, I believe we should.
| ### Llama-cpp backend specific commands | ||
|
|
||
| ```shell | ||
| ilab model serve llama-cpp --model <PATH> --llama-cpp-gpu-layers 4 --llama-cpp-max-ctx-size 1024 |
There was a problem hiding this comment.
A point I've raised before, I'm not sure if it makes sense to prefix everything with --llama-cpp if we know we are using the llama-cpp backend.
There was a problem hiding this comment.
The reason I made it so specific is that I'm scarred by the flags for training that exist in the codebase as of 6/7/24. It's not clear which training flags belong with the mac implementation of training vs the linux implementation and on top of that the flags are very similar.
An example of how the flags could be mixed up. Current ilab serve with llama-cpp has a flag for max-ctx-size. vllm has a flag for model context length called --max-model-len (https://docs.vllm.ai/en/latest/serving/openai_compatible_server.html#command-line-arguments-for-the-server).
We could do the following possibilities:
-
Have a flag for
--max-model-lenand--max-ctx-sizesimilar to how we have it in currentilab train. I think this option is hardest on the user. -
Have one flag for context size called
--max-ctx-sizewhose value is filtered down to each backend. An issue then becomes what do you do with flags that one backend has that one doesn't. Also you've now added this flag into the set of flags that both backends can use.
In my current design I propose we make backend agnostic flags those flags have nothing to do with the configuration of the backend itself and more for the configuration of ilab and what the backends are operating on (like the model path).
- Prefix all flags related to a backend with the backend name. I think this is the most clear for the users with the downside that it's more verbose.
@cdoern what do you think about how the flags for training might look. Training will have a similar set of possible subcommands with numerous options.
There was a problem hiding this comment.
In my current design I propose we make backend agnostic flags those flags have nothing to do with the configuration of the backend itself and more for the configuration of ilab and what the backends are operating on (like the model path).
+1 this is the right approach to create and use generic flags yet still have a mechanism for provider specific flags. I also liked how in your draft PR you had a generic --vllm-args concept in there where it's just a passthrough.
There was a problem hiding this comment.
I think provider specific flags are a good idea from a UX pov because it means naturally users won't try to apply a flag specific to one backend to another. (Again thinking of my ffmpeg nightmares ;-) )
There was a problem hiding this comment.
I'm open to building generic flags, but it feels like we're creating a third layer of abstraction, which might introduce some ambiguity. Users will have to juggle raw flags from llama-cpp, as well as flags from ilab, which could complicate their mental model when writing commands. However, I do like the idea of re-using sensible flags and handling the backend mapping internally.
In the end, we could adopt two strategies:
- Backend-Agnostic Flags: Select the most common flags that make sense across multiple backends.
- Backend-Specific Flags: Use --vllm-args (or similar) for backend-specific configurations.
There might still be issues, such as someone passing ilab model serve --backend llama-cpp --llama-cpp-max-ctx-size 1 --llama-cpp-args "llama-cpp-max-ctx-size 2". However, we can address this through proper validation and error messaging.
There was a problem hiding this comment.
After doing some more research I'm removing the backend specific prefixes from the backend specific flags. The way we're using click for subcommands, flags between backends cannot be shared.
So running ilab model serve vllm --help would yield a different message than ilab model serve --help and ilab model serve llama-cpp --help.
For this reason I'm not worried about users mixing up arguments as long as there are subcommands.
This is another reason why I believe going subcommands instead of having a --backend flag to choose the backend.
| ### Llama-cpp backend specific commands | ||
|
|
||
| ```shell | ||
| ilab model serve llama-cpp --model <PATH> --llama-cpp-gpu-layers 4 --llama-cpp-max-ctx-size 1024 |
There was a problem hiding this comment.
I think provider specific flags are a good idea from a UX pov because it means naturally users won't try to apply a flag specific to one backend to another. (Again thinking of my ffmpeg nightmares ;-) )
russellb
left a comment
There was a problem hiding this comment.
I left some minor comments.
It seems the only thing blocking this is whether backend is a positional argument or a flag.
If it was clear we’d have a bunch of backend specific options, I’d lean toward a positional argument to explicitly separate the set of options for each backend.
It seems the doc proposes a generic pass through “—vllm-args” so it seems like differences will be minimal and clear enough.
If I had to pick I’d go with the flag but I’m supportive of either one. We just need to resolve this quickly so implementation can proceed.
n1hility
left a comment
There was a problem hiding this comment.
Temp requesting changes just to verify the backend vs subcommand discussion
0f87719 to
a348dc5
Compare
russellb
left a comment
There was a problem hiding this comment.
I'm good with the direction in general, though I have some comments on the syntax of the --backend-args option.
|
@leseb @russellb @n1hility @mairin Thanks for moving this forward., I think One question I have is do we need to be able to provide the ability to turn all of the knobs in the CLI that we could in a config file? AKA does every parameter need the ability to be set in the config and on the CLI? I'm guessing the answer is yes, but if that's not that case why don't we just have:
At least with this approach there would be one source of truth, the implementation would be clean and simple, and we would not be passing and parsing flags within flags. I've never seen another project with flags similar to my original |
|
I would say yes for ease of use if you want to override one parameter or
whatnot in the config file.
|
|
The other thing I would say, is just passing the config file might make the world simpler if we we do default config setting with profiles like @cdoern has proposed. The user would only have to focus their attention on the config file. Charlie if you have any thoughts, please chime in. |
|
@alimaredia @mairin @n1hility @russellb, I'd like to hold off on discussing Much appreciated. |
Sure but what we merge needs to reflect what we have consensus on. So to defer the backend args we need to revise the text from being a requirement to being informative. |
This commit introduces a new design document for the [`ilab model serve`] command, detailing its functionality to serve models using different backends, specifically `llama-cpp` and `vllm`. It outlines the command structure, including backend-specific flags and arguments, and proposes a testing strategy for new engine integrations. Co-authored-by: Sébastien Han <seb@redhat.com> Co-authored-by: Jason Greene <jason.greene@redhat.com> Signed-off-by: Ali Maredia <amaredia@redhat.com>
264274a to
fc7c694
Compare
leseb supports the changes
|
I won't hold anything up but I don't agree with making --backend a flag. I have read back through comments here several times and do not see a rationale for this change. I think it is important to understand why we are going from subcommand to flag, as the subcommand approach was discussed extensively in brainstorming this devdoc and reviewed verbally with multiple folks - even rhtdan felt it was the way to go. I would feel better if I understood why we are going to a flag from a subcommand. My core concern is from the user experience end. The user experience for this project is incredibly important and if we can have a delightful one it will be a differentiator. However, I am not considering other things like what limitations click might have. I think having --backendargs as a catch all and starting with that for the implemetation is a great idea Again, I will not hold this one up bc we need to complete this and have smtg working and that is the most important. |
|
Since some of the comments are not visible anymore after a push, I'd like to summarize why using a
As a side note: I'd argue that the only parameter that should be positional is the model path, as it's essential. I'd to reference the guide from CLIG.dev:
In conclusion, using Thanks! @mairin I hope this will change your mind :). Are you keeping your request change to express your disagreement with the latest proposal? I'm seeking clarification because your last comment said you won't hold this up. I'm fine if you want to keep it and have someone else merge if this gets other approvals. |
|
Sébastien and I had a call this morning and I am on board with this design now. The rationale had been squashed and he posted it above, and I also misunderstood part of how the flag was proposed to work. I am on board with this now. My main concern was making sure users do not try to use on set of arguments they learned to use with llamacpp and try to xfer them to a vllm or other backend and have a poor experience. Some things we discussed is capturing when that happens and erroring out gracefully, or letting the user know it won't work if we can detect that's going on (with an out to override and try it anyway), and also giving suggesting based on what the argument for backend A is with the equivalent argument for backend B. |
This commit introduces a new design document for the [
ilab model serve] command, detailing itsfunctionality to serve models using different backends, specifically
llama-cppandvllm. It outlines the command structure, includingbackend-specific flags and arguments, and proposes a testing strategy
for new engine integrations.
Co-authored-by: Sébastien Han seb@redhat.com
Signed-off-by: Ali Maredia amaredia@redhat.com