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

introduce additional_arguments in config.yaml #111

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jul 3, 2024

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 key ilab commands.

additional_arguments allows ilab to implicitly support more complex flags from the libraries it calls without adding them directly to the config.

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>
Copy link
Contributor

@leseb leseb left a 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.

Copy link
Member

@hickeyma hickeyma left a 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"...]
Copy link
Member

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.

@cdoern
Copy link
Contributor Author

cdoern commented Jul 5, 2024

@leseb

I don't think we should "hide" any options. If we do, how will users discover them?

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.

@leseb
Copy link
Contributor

leseb commented Jul 8, 2024

@leseb

I don't think we should "hide" any options. If we do, how will users discover them?

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.

@nathan-weinberg
Copy link
Member

@cdoern what is going on with this?

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.

5 participants