-
Notifications
You must be signed in to change notification settings - Fork 39
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
✨ Clean up and simplify config management #320
Conversation
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.
Looking good overall, but I did have a few questions.
- Lets verify end user can see/edit the
config.toml
used, concern mostly with workflow of how an end user sees the configured model and may change this model. - Let's consider keeping ROADMAP.md at the main project root.
- I see the logs directory is no longer part of compose.yaml, unsure if that is intended.
build/entrypoint.sh
Outdated
# If a custom config is specified, use it | ||
if [[ -f /podman_compose/build/config.toml ]]; then | ||
printf "Using custom config.toml\n" | ||
cp /podman_compose/build/config.toml /kai/kai/config.toml |
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.
Will an end user running podman compose up
have access to the config.toml
on their local disk so they can see the settings and adjust?
The usecase we want to satisfy is to allow the end user to:
- View the
config.toml
from local disk - Change settings on local disk to modify configured models
I'm not sure from a quick review of this PR if there is a mechanism for the end user to view/edit the config.toml
that is used. My current impression is we are copying it into the container at build time. I see how that helps for running with the default configurations and I see the intent to override via environment variables, but unsure about how a user can modify the config.toml.
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.
The user can easily adjust their config.toml
. The way it works in this PR is:
- When the container is built, the
kai/config.toml
that is present in the repo is copied over to the container - When the container is run, it first checks if the file
build/config.toml
exists. If it does, it copies it over to the container.
Thus, the user can specify their config via build/config.toml
. There is an example_config.toml
also present in that directory. We could even add example_stable_config.toml
or something so the user has a config that can match their version. We can change the location that entrypoint.sh
looks for if it makes more sense for the user to specify their custom config elsewhere.
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 updated how this works to make more sense. See my comment below.
I updated how config priority is done. It's now done in the following way (higher overrides lower):
Upon further evaluation, we could merge |
c325fda
to
3dc02e3
Compare
Is it possible to swap this and allow Environment variables to have top precedence? When I think of
Reason being is a user who is using
I assume in time they will understand we also have a configuration file they can view/edit at
I didn't see a When I see I'm not advocating we want/need that, just sharing what went through my head when I saw this. I don't see the |
…arted.md) Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
99bdc0f
to
5689d0c
Compare
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Going to hold off on merging due to #332 preventing me from testing. It was working earlier but after deleting the logs directory, which was having permission errors on bare metal, I can no longer run If anyone else can verify everything works then I'm good to merge. |
Signed-off-by: JonahSussman <sussmanjonah@gmail.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.
Confirmed working on MacOS
Closes #264 |
Confirmed working on Ubuntu 24.04.1 |
Config is now sourced from the environment as well as the config.toml via pydantic-settings. This makes Kai align more with 12-factor config principles.
pydantic-settings allows for easy config overrides (higher overrides lower):
podman compose up
, the config file inbuild/config.toml
is supplied as a command line argument.kai/config_local.toml
. This is for quick changes when running on bare metal.kai/config_default.toml
. Sane defaults.Task list: