-
Notifications
You must be signed in to change notification settings - Fork 30
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
introduce additional_arguments in config.yaml #111
base: main
Are you sure you want to change the base?
Conversation
additional_arguments allows ilab to implicitly support more complex flags from the libraries it calls without adding them directly to the config Signed-off-by: Charlie Doern <cdoern@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this proposal. The configuration file should expose all the available configuration options in the software. Unless we are embedding other software (like we do with vLLM), I don't think we should "hide" any options. If we do, how will users discover them?
Another approach would be to follow the common practice of including all configuration options in the config files but commenting them out, along with descriptions of what they are, what they do and their default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cdoern for pushing this proposal.
If I am reading it correctly, you are trying to decrease the configuration properties to make the configuration file more manageable. If this is so, would it be helpful to make a table of config items per capability (generation, training, etc.) and then have columns like Property Name
, To Maintain
, Reason
. That way the reader can understand the properties being maintained or discarded and why.
llm_family: '' | ||
model_path: models/merlinite-7b-lab-Q4_K_M.gguf | ||
served_model_name: "merlinite" | ||
additional_arguments: ["--block-size=16", "--dtype=fp8"...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this work if there are multiple commands callable from one config section? Would we not need the additional_arguments name to indicate where in particular the additional args are being passed through to?
With serving, you could choose to use the same args field to pass values to vllm or llama_cpp or another serving technology. But If there are two calls to make to vllm or llama_cpp, it becomes less clear.
We have had a few conversations in the past week about the ever increasing complexity of the libraries in instructlab. Its not realistic to expect every option added in the libraries to match 1:1 with cfg options and flags in instructlab, but ilab should also be able to support them all one way or another. If we are operating under the assumption ilab is the main point of entry into the libraries, it should support all the complex options the libraries have to offer. However, having these all spelled out in the config is pretty bad UX. This would allow us to choose which ones the user would most commonly need to edit while also supporting the more complicated ones, just implicitly. |
Ok, that sounds reasonable to me then, I missed that during my initial review. Thanks for the clarification. |
@cdoern what is going on with this? |
This dev-doc introduces
ilab-config-structure.md
a document meant to discuss how we deal with nested classes in the config. Additionally this document discusses implicit vs explicit arguments that have been previously added in different ways in keyilab
commands.additional_arguments
allows ilab to implicitly support more complex flags from the libraries it calls without adding them directly to the config.