Modify the Eval config to operate like training and serving - #1564
Conversation
e57c91a to
77919b5
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
There seems to be some debate here over the config -> flag overrides -> libraries design path I have been implementing across the CLI. This is the design language I am trying to implement for each CLI cmd. I see how serving+eval is a special case because some options are only applicable to a certain backend. However, we can probably safely assume the amount of options per backend will expand over time. I think hiding the max-ctx-size and backend flags and instead pulling them from the cfg is dangerous as it violates the design language I am trying to introduce. I know in many scenarios the user won't need to set these however, consider the following scenario I stated above: ` The reason I did this is because of the flow we are using in the cfg. if user has the following serve config: serve: but they change the backend manually by editing their config to: serve: They run a server and a chat process and are as happy as can be. However, the --backend evaluate flag serves as a way to NOT modify their config for all other commands (serve, chat, gen, etc) but allows them to manually set --backend=vllm just for this cmd. I think hidden checks are useful but it is SO valuable to allow the user to override their options in the config.yaml from a UX perspective. We are taking this approach everywhere else so I feel pretty strongly about at least exposing it here for power users. I would be ok with some of these being hidden, but for the sake of continuity over the CLI I don't think pulling things directly out of the cfg and NOT having a flag override is something we can do |
leseb
left a comment
There was a problem hiding this comment.
I'm okay with the overall approach, though I do prefer having auto-detection in place. It makes sense to have --backend for overriding.
Auto-detection is important because, without it, we risk an internal failure of the serving backend if the model and backend are incompatible. Auto-detection would prevent such issues.
This can be worked in a followup PR.
@cdoern what do you think?
|
This pull request has merge conflicts that must be resolved before it can be |
danmcp
left a comment
There was a problem hiding this comment.
Waiting for rebase before approving again
this new flag is defaulted from the _serve part of the config.yaml. If the user wants to, they can override the config options by using the new flag. Also removed two edits to the config obj. Rather than editing the config, warn the user if the options in the config may be incompatible Signed-off-by: Charlie Doern <cdoern@redhat.com>
Signed-off-by: Charlie Doern <cdoern@redhat.com>
addressed comments, and nathan is on PTO. Please feel free to follow up if I missed anything!
Training and Serving now operate under the following workflow when it comes to the config.yaml
ilab config initilab model serveorilab model trainthe values on the cfg are flattened and used as the defaults for the cmd flagsEval is a special case as it has its own cfg section but also needs to use bits from the serve portion of the cfg.
This PR flattens and includes the serve cfg in the eval default flag map. In order to make this work properly I added
--backendas a flag for eval. This serves as an override depending on if the user intends to use vllm or llama-cpp.The current code overrides some stuff manually in the serve config, I don't think we should be in the buisness of doing this behind the scenes. If the users wants to change their max-ctx-size or max_workers, they can do so in the cfg or in the flags