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

Change config precedence so env vars are king #413

Closed
wants to merge 1 commit into from

Conversation

dymurray
Copy link

@dymurray dymurray commented Oct 4, 2024

fixes #399

Signed-off-by: Dylan Murray <dymurray@redhat.com>
@dymurray
Copy link
Author

dymurray commented Oct 4, 2024

After talking to @JonahSussman We may prefer config.toml taking precedence. If we do we just need to remove some of the defaults in config_example.toml to resolve the podman issues.

Removing some of the defaults in the example resolves this most of the time, but not 100%.

@shawn-hurley
Copy link
Contributor

I am confused on the next steps.

Should we close this and re-open with the config_example.toml changes?

@dymurray
Copy link
Author

dymurray commented Oct 7, 2024

@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?

@JonahSussman
Copy link
Contributor

JonahSussman commented Oct 7, 2024

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:

export KAI__LOG_LEVEL = INFO

and you did this in Python in a Python test case:

config = KaiConfig(log_level="DEBUG")
assert config.log_level == "DEBUG"

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:

  1. Remove the build/config.toml file entirely, and the lines from entrypoint.sh that pass it in as a command line argument.
  2. Ask users to configure their instance with a .env file, and pass it in to podman compose with --env-file <file> (pydantic-settings should support .env files natively, but podman just might make it part of the environment so it doesn't matter. Haven't tested this)
  3. (Optional) Somehow expose command line arguments to the user outside of podman compose up so the user can override things to their heart's content

Thoughts?

@JonahSussman
Copy link
Contributor

JonahSussman commented Oct 7, 2024

To clarify, the crux of the issue is that you need to have some way to pass in config through podman compose to the main program. Whether that's a build/config.toml, environment variables, or a .env file, there's no way to avoid it.

We would basically be swapping out the build/config.toml approach with a .env approach. So the config file:

[models]
provider = "ChatIBMGenAI"

[models.args]
model_id = "mistralai/mixtral-8x7b-instruct-v01"

would become:

KAI__MODELS__PROVIDER = "ChatIBMGenAI"
KAI__MODELS__ARGS = '{"model_id": "mistralai/mixtral-8x7b-instruct-v01"}'

@shawn-hurley
Copy link
Contributor

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?

@dymurray
Copy link
Author

Closing in favor of a different approach

@dymurray dymurray closed this Oct 11, 2024
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.

Precendence of environment variables or configuration file values is causing unexpected issues
3 participants