-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: add support for loading different config.yaml files #609
feat: add support for loading different config.yaml files #609
Conversation
def extend_config(self, config_path): | ||
if self.debug_mode: | ||
print(f'Extending configuration from `{config_path}`') | ||
|
||
config = get_config(config_path) | ||
self.__dict__.update(config) |
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.
Moving this into a class method allows it to be called programmatically so that developers can load configs when using interpreter
in Python scripts, as well.
There's an example of this in test_interpreter.py
with open(user_config_path, 'r') as file: | ||
user_config_path = os.path.join(get_storage_path(), config_filename) | ||
|
||
def get_config_path(path=user_config_path): |
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.
This method had to get a little more complex, unfortunately, but it now handles loading by name from the default config directory, by relative path from the current directory, or by absolute path.
If the config file path doesn't exist it will create it and pre-populate it with the default config.yaml
.
tests/test_interpreter.py
Outdated
interpreter.temperature = 0 | ||
interpreter.auto_run = True | ||
interpreter.model = "gpt-3.5-turbo" | ||
interpreter.debug_mode = False |
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.
Now that we're testing configuration overrides by loading a custom config file, we need to make sure that our setup function resets to the expected parameters for our tests.
9b75b10
to
8de533f
Compare
8de533f
to
033f43d
Compare
This adds a --config_file option that allows users to specify a path to a config file or the name of a config file in their Open Interpreter config directory and use that config file when invoking interpreter. It also adds similar functionality to the --config parameter allowing users to open and edit different config files. To simplify finding and loading files I also added a utility to return the path to a directory in the Open Interpreter config directory and moved some other points in the code from using a manually constructed path to utilizing the same utility method for consistency and simplicity.
033f43d
to
b0fb3d7
Compare
@@ -54,7 +71,7 @@ def test_hello_world(): | |||
{"role": "assistant", "message": hello_world_response}, | |||
] | |||
|
|||
""" | |||
@pytest.mark.skip(reason="Math is hard") |
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.
@KillianLucas I updated this to use pytest
's built-in skip
functionality instead of just commenting it out.
|
||
# Check for update | ||
if not self.local: | ||
# This should actually be pushed into the utility | ||
if check_for_update(): | ||
display_markdown_message("> **A new version of Open Interpreter is available.**\n>Please run: `pip install --upgrade open-interpreter`\n\n---") | ||
|
||
def extend_config(self, config_path): |
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.
We could also call this load_config
instead of extend_config
and then it would be more clear what's going on when you call interpreter.load_config()
via Python.
default_config_path = os.path.join(parent_dir, 'config.yaml') | ||
|
||
# Copying the file using shutil.copy | ||
new_file = shutil.copy(default_config_path, path) |
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 was thinking that we might want to exclude the system_message
from the new config.yaml
file in order to have people using the default message as things are upgraded unless they really want to customize it.
Right now any changes we make to the default system_message
in interpeter/config.yaml
will be overwritten by the system_message
in the user's config.yaml
file, which means if we need to push out some update to the underlying prompt engineering that powers Open Interpreter, users who have already generated a config.yaml
file will not receive it unless they manually update their config.yaml
with the latest system prompt.
It might also be good to introduce a property like append_to_sysem_message
or appended_system
message that will allow the user to just specify some instructions they want to concatenate onto the end of our default system_message
.
If either of those are approaches that we want to take, I'd be happy to submit a follow-up PR that addresses them.
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 was thinking having them be able to change it would be a good idea actually, as if they are using it for specific purposes, then altering the prompt slightly for their use case, would somewhere make it more efficient wouldn't it?
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 still think providing the ability to change it is a good thing, but I think it should be an opt-in rather than opt-out operation.
Right now running interpreter --config
generates a config.yaml
file with the current default system_prompt
from interpreter/config.yaml
.
If we push out a new update that includes some changes to the system_prompt
, users who have already generated a config.yaml
will not receive those changes unless they manually update their system_prompt
. This is fine, but I think users's should have to explicitly choose to overwrite the system_prompt
property rather than it happening automatically.
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.
Makes sense actually, didn't think of when we would update the system_prompt from our ends.
Tested on windows looks good to me, @KillianLucas let's have a check from your end too, this seems too good to not merge!! |
…ore-configs feat: add support for loading different config.yaml files Former-commit-id: 8ec9492 Former-commit-id: de6709c991591511a5c0241afec59ab38ef60a0d Former-commit-id: 7d27d85296b8e25ea5fb2c870f86c7d4137b17f9 [formerly 84035ab762716fa930471a1e2fc5db91f06f6925] Former-commit-id: ac05d9cbdc9f270d96c8dd1504eca19b6e92e45e
…ore-configs feat: add support for loading different config.yaml files
Describe the changes you have made:
This adds a
--config_file
option that allows users to specify a path to a config file or the name of a config file in their Open Interpreter config directory and use that config file when invokinginterpreter
.It also adds similar functionality to the
--config
parameter, allowing users to open and edit different config files by passing the path with--config_file
.To simplify finding and loading files, I also added a utility to return the path to a directory in the Open Interpreter config directory and moved some other points in the code from using a manually constructed path to utilizing the same utility method for consistency and simplicity.
I also added a test that uses the
interpreter.extend_config()
method to validate that configuration files can be loaded.Note: This functionality has become more important as I try to help folks with Issues here on GitHub and in the Discord. Because my own
config.yaml
is customized, it's hard to test with the default config without manually renaming/deleting my customized config first.Demo
Testing
gh pr checkout https://github.com/KillianLucas/open-interpreter/pull/609
poetry run interpreter --config --config_file config.testing.yaml
to create a new config filemodel
is the easiest one to see the effects ofpoetry run interpreter --config_file config.testing.yaml
to load your customized config filepoetry run interpreter --config
to see the default config filepoetry run interpreter
to load the default config filepoetry run interpreter --config_file ./tests/config.test.yaml
to load a config file from a relative path (this is the config file that ourpytest
test for config loading uses)You can also test with local and absolute config file paths.
Reference any relevant issue (Fixes #000)
I have tested the code on the following OS:
AI Language Model (if applicable)