-
Notifications
You must be signed in to change notification settings - Fork 33
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
Change config precedence so env vars are king #413
Conversation
Signed-off-by: Dylan Murray <[email protected]>
After talking to @JonahSussman We may prefer Removing some of the defaults in the example resolves this most of the time, but not 100%. |
I am confused on the next steps. Should we close this and re-open with the config_example.toml changes? |
@shawn-hurley personally I would like to see this merged. I don't think that changing the example file is enough to resolve this the more I play with it. If I switch between dev env and the compose workflow this will still break with the database config values set. I feel strongly that env vars taking highest precedence is the most common flow of configuration across languages. Am I wrong? |
I'm down to make env variables king, but the PR currently works by overriding the init function of the class. Take, for example, this environment:
and you did this in Python in a Python test case:
It will fail in some environments and succeed in others. Thus, I don't think we should mess with the precedence. Instead, I think we should:
Thoughts? |
To clarify, the crux of the issue is that you need to have some way to pass in config through We would basically be swapping out the [models]
provider = "ChatIBMGenAI"
[models.args]
model_id = "mistralai/mixtral-8x7b-instruct-v01" would become:
|
I don't know if I would want to remove a config file. Env vars in the Windows world are a lot harder to work, IIRC. In Podman compose, can you not mount the configuration file? |
Closing in favor of a different approach |
fixes #399