-
Notifications
You must be signed in to change notification settings - Fork 45
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
Array-Based Configuration for LLM Providers with Target Flag #74
Comments
I had a free hour and looked around in the code and tried to sketch out a PoC for that with compatibility in mind.
However, I don't think it's that trivial with the current architecture. Viper can handle conf + flag + env with priorities (overrides), but with the introduction of an extra target list layer, after viper is done with parsing the way it does their job, there's no way to tell if given value was provided by config (root value), env variable, or cli flag. Ideally the priority would be:
So for example for model with pseudo code
The problem with this approach, by the time viper is done with parsing, we can't tell where the value of I didn't use viper for a long time (probably 4 years ago last time), but I don't think there is a way to inject this logic in there in a sane way. If we drop the top level configuration it's still tricky to get this working with viper for the array as it tries to map env and flag for a single value, but if there are 2 entries in that list, it has to map note: I didn't pick up this as "oh I want to do this", because I have very limited free time these days (I would if I have more time). I just tried to play around and check the code to determine the complexity of this issue and decide if I can do it in a few hours on a weekend, but I don't think I can, it's definitely more than a few hours 😆 |
I tried to think about an alternative solution. This feature can be introduced with less work if it's not a single configuration file.
Effectively just swapping the configuration file. It's not as elegant as having a single structure configuration file, but can handle multiple providers still with an easy switch and a user can set a default with a config file or symlink given config. A very ugly monkey patch to do that: diff --git a/cmd/chatgpt/main.go b/cmd/chatgpt/main.go
index 80e9210..5e56157 100644
--- a/cmd/chatgpt/main.go
+++ b/cmd/chatgpt/main.go
@@ -389,12 +389,19 @@ func initConfig(rootCmd *cobra.Command) (config.Config, error) {
// Set default name for environment variables if no config is loaded yet.
viper.SetDefault("name", "openai")
+ _ = rootCmd.ParseFlags(os.Args)
+
+ configName := "config"
+ if target := rootCmd.Flag("target").Value.String(); target != "" {
+ configName += "." + target
+ }
+
// Read only the `name` field from the config to determine the environment prefix.
configHome, err := internal.GetConfigHome()
if err != nil {
return config.Config{}, err
}
- viper.SetConfigName("config")
+ viper.SetConfigName(configName)
viper.SetConfigType("yaml")
viper.AddConfigPath(configHome)
@@ -607,6 +614,7 @@ func setCustomHelp(rootCmd *cobra.Command) {
printFlagWithPadding("-c, --config", "Display the configuration")
printFlagWithPadding("-v, --version", "Display the version information")
printFlagWithPadding("-l, --list-models", "List available models")
+ printFlagWithPadding("-t, --target", "Set target")
printFlagWithPadding("--list-threads", "List available threads")
printFlagWithPadding("--delete-thread", "Delete the specified thread (supports wildcards)")
printFlagWithPadding("--clear-history", "Clear the history of the current thread")
@@ -660,6 +668,7 @@ func setupFlags(rootCmd *cobra.Command) {
rootCmd.PersistentFlags().StringVar(&threadName, "delete-thread", "", "Delete the specified thread")
rootCmd.PersistentFlags().BoolVar(&showHistory, "show-history", false, "Show the human-readable conversation history")
rootCmd.PersistentFlags().StringVar(&shell, "set-completions", "", "Generate autocompletion script for your current shell")
+ rootCmd.PersistentFlags().StringP("target", "t", "", "Target to use")
}
func setupConfigFlags(rootCmd *cobra.Command, meta ConfigMetadata) { I can clean it up and file a PR for this approach if you prefer that and you don't have time to do that. It can solve some of the pain points this issues is trying to resolve, but it's not as elegant as having a nice and clean configuration file. A background why I perused the "targets" path and not "providers" path in both cases:
|
@yitsushi first of all, thank you so much!! I didn't consider these viper limitations. Let me play with viper real quick to see if I can get an array based config to work. If not, I think your patch would make a lot of sense. Just have different configuration files and pick one with a target flag. That solves the problem as well. Great idea! |
I haven't had much time just yet, but perhaps something like this would work?
And do the parsing like this:
Will report back once I have some time to implement this. |
package main
import (
"bytes"
"fmt"
"os"
"github.com/spf13/viper"
)
type Provider struct {
Name string `mapstructure:"name"`
APIKey string `mapstructure:"api_key"`
Model string `mapstructure:"model"`
MaxTokens int `mapstructure:"max_tokens"`
ContextWindow int `mapstructure:"context_window"`
}
type Config struct {
Providers []Provider `mapstructure:"providers"`
}
const exampleConfig = `---
providers:
- name: openai
api_key: 123
model: gpt-4o
- name: openai
api_key: 123
model: gpt-4o-mod
`
func main() {
config := Config{}
viper.SetConfigType("yaml")
viper.SetEnvPrefix("example")
viper.BindEnv("max_tokens")
viper.ReadConfig(bytes.NewBuffer([]byte(exampleConfig)))
viper.ReadInConfig()
if err := viper.UnmarshalExact(&config); err != nil {
fmt.Fprintf(os.Stderr, "[UnmarshalExact] error: %v\n", err)
os.Exit(1)
}
if err := viper.Unmarshal(&config); err != nil {
fmt.Fprintf(os.Stderr, "[Unmarshal] error: %v\n", err)
os.Exit(1)
}
fmt.Fprintf(os.Stdout, "%#v\n", config)
for name, provider := range config.Providers {
fmt.Fprintf(os.Stdout, "%d: %#v\n", name, provider)
}
} In this case,
It's the same issue with map: package main
import (
"bytes"
"fmt"
"os"
"github.com/spf13/viper"
)
type Provider struct {
Name string `mapstructure:"name"`
APIKey string `mapstructure:"api_key"`
Model string `mapstructure:"model"`
MaxTokens int `mapstructure:"max_tokens"`
ContextWindow int `mapstructure:"context_window"`
}
type Config struct {
Providers map[string]Provider `mapstructure:"providers"`
}
const exampleConfig = `---
providers:
option-one:
name: openai
api_key: 123
model: gpt-4o
option-two:
name: openai
api_key: 123
model: gpt-4o-mod
`
func main() {
config := Config{}
viper.SetConfigType("yaml")
viper.SetEnvPrefix("example")
viper.BindEnv("max_tokens")
viper.ReadConfig(bytes.NewBuffer([]byte(exampleConfig)))
viper.ReadInConfig()
if err := viper.UnmarshalExact(&config); err != nil {
fmt.Fprintf(os.Stderr, "[UnmarshalExact] error: %v\n", err)
os.Exit(1)
}
if err := viper.Unmarshal(&config); err != nil {
fmt.Fprintf(os.Stderr, "[Unmarshal] error: %v\n", err)
os.Exit(1)
}
fmt.Fprintf(os.Stdout, "%#v\n", config)
for name, provider := range config.Providers {
fmt.Fprintf(os.Stdout, "%s: %#v\n", name, provider)
}
}
Default value can be handled with extra top level fields, but that doesn't handle provider level ENV variables, or even cli flags: package main
import (
"bytes"
"fmt"
"os"
"github.com/spf13/viper"
)
type Provider struct {
Name string `mapstructure:"name"`
APIKey string `mapstructure:"api_key"`
Model string `mapstructure:"model"`
MaxTokens int `mapstructure:"max_tokens"`
ContextWindow int `mapstructure:"context_window"`
}
type Config struct {
MaxTokens int `mapstructure:"max_tokens"`
Providers map[string]Provider `mapstructure:"providers"`
}
const exampleConfig = `---
max_tokens: 100
providers:
option-one:
name: openai
api_key: 123
model: gpt-4o
option-two:
name: openai
api_key: 123
model: gpt-4o-mod
option-three:
name: openai
api_key: 123
model: gpt-4o-mod
max_tokens: 1000
`
func main() {
config := Config{}
viper.SetConfigType("yaml")
viper.SetEnvPrefix("example")
viper.BindEnv("max_tokens")
viper.ReadConfig(bytes.NewBuffer([]byte(exampleConfig)))
viper.ReadInConfig()
if err := viper.UnmarshalExact(&config); err != nil {
fmt.Fprintf(os.Stderr, "[UnmarshalExact] error: %v\n", err)
os.Exit(1)
}
if err := viper.Unmarshal(&config); err != nil {
fmt.Fprintf(os.Stderr, "[Unmarshal] error: %v\n", err)
os.Exit(1)
}
// Set global max token if local is not defined.
for name, provider := range config.Providers {
current := config.Providers[name]
if provider.MaxTokens <= 0 {
current.MaxTokens = config.MaxTokens
}
config.Providers[name] = current
}
fmt.Fprintf(os.Stdout, "%#v\n", config)
for name, provider := range config.Providers {
fmt.Fprintf(os.Stdout, "%s: %#v\n", name, provider)
}
}
Even if that's solved with a custom Decoder for viper for the env parser, flags are still a question. (an hour later 😆 ) After spent some time on it, if the logic is gets a cut and it has two parts:
Then a workflow can be applied like:
That's a lot of work, but probably works, and doesn't really break anything (we there is a default empty provider if A PoC Go code: package main
import (
"bytes"
"fmt"
"os"
"strings"
"github.com/spf13/pflag"
"github.com/spf13/viper"
)
type Provider struct {
Name string `mapstructure:"name"`
APIKey string `mapstructure:"api_key"`
Model string `mapstructure:"model"`
MaxTokens int `mapstructure:"max_tokens"`
ContextWindow int `mapstructure:"context_window"`
}
type Config struct {
MaxTokens int `mapstructure:"max_tokens"`
Provider string `mapstructure:"provider"`
Providers map[string]Provider `mapstructure:"providers"`
}
const exampleConfig = `---
max_tokens: 100
provider: option-one
providers:
option-one:
name: openai
api_key: 123
model: gpt-4o
option-two:
name: openai
api_key: 123
model: gpt-4o-mod
option-three:
name: openai
api_key: 123
model: gpt-4o-mod
max_tokens: 1000
`
func main() {
config := Config{}
pflag.Int("max-tokens", 0, "max tokens")
pflag.String("provider", "", "provider to use")
pflag.Parse()
// Config file.
fConfig := viper.New()
fConfig.SetConfigType("yaml")
fConfig.ReadConfig(bytes.NewBuffer([]byte(exampleConfig)))
// Runtime Config (env, flag).
rConfig := viper.New()
rConfig.AutomaticEnv()
rConfig.SetEnvPrefix("example")
rConfig.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
rConfig.BindPFlags(pflag.CommandLine)
if err := fConfig.UnmarshalExact(&config); err != nil {
fmt.Fprintf(os.Stderr, "[UnmarshalExact] error: %v\n", err)
os.Exit(1)
}
// Set global max token if local is not defined.
for name, provider := range config.Providers {
current := config.Providers[name]
if provider.MaxTokens <= 0 {
current.MaxTokens = config.MaxTokens
}
config.Providers[name] = current
}
if value := rConfig.GetString("provider"); value != "" {
config.Provider = value
}
provider, ok := config.Providers[config.Provider]
if !ok {
fmt.Fprintf(os.Stderr, "provider %s not found\n", config.Provider)
os.Exit(1)
}
// Fill in
if value := rConfig.GetInt("max-tokens"); value > 0 {
provider.MaxTokens = value
}
fmt.Fprintf(os.Stdout, "%#v\n", provider)
} Output:
(I know I went with the map again, but it should work the same way for an array too, instead of a map lookup, it's a loop, it was simpler now to keep the code smaller) Edit: |
Oh my, this is great! (sorry I am on vacation and not online as much as usual). It's does smell though, you're totally right. I still really like your initial solution with the formatted filenames, ie: Do you think the name could be unique? Or what would be a good argument against that?
I'm sorry in case I missed your explanation. |
Ugh, what a headache to keep it backwards compatible as well :/. I'd probably end up writing a migration "script" instead. |
I pushed this super hack: https://github.com/kardolus/chatgpt-cli/tree/providers Basically just made it so it compiles. Totally doesn't work though, but this could be a good starting point to figure out if we can get this to work in the first place (by updating main.go). If we can get it to work e2e I can do all the dirty work (ie. update all the tests etc). |
The main benefit of a map instead of a slice:
I think that's it. The reason the second one can be handy: But really, that's it. If it's a slice (array), there has to be a loop to find the right target configuration, and if name is used for that, the user is forced to define what's the ENV to use for that provider, as no one wants to define environment variables like For the examples: providerConfig, ok := conf.Providers[providerName]
if !ok {
// ... not found error
}
// vs
var providerConfig ProviderConfig
for _, provider := range conf.Providers {
if provider.Name == providerName {
providerConfig = conf.Providers
break
}
}
if providerConfig.Name == "" {
// ... not found error
} After that, in both cases:
So it's really the same. |
Ah I see I see. Thanks! |
Oh my, this is so tricky. I am leaning towards your initial solution which is simply using filenames with clever extensions. It seems user friendly enough. |
Problem
Currently, the configuration for the ChatGPT CLI uses a singular configuration format for LLMs and models. This works well if you're using only one LLM at a time, but it becomes cumbersome when switching between different providers (OpenAI, Perplexity, Llama, etc.) or models.
For example, the current configuration looks like this:
When switching between providers or models, users have to either edit the configuration file or rely on environment variables. This makes switching between multiple setups more complicated and less efficient.
Proposed Solution
Introduce Array-Based Configuration for LLMs:
Convert the current configuration from a single setup to an array-based format where multiple configurations for different LLMs and models can be stored.
Example:
Add a
--target
Flag to Dynamically Select a Configuration:Add a
--target
flag that allows users to select which configuration (provider and model) to use for a specific command.Example:
This way, users can quickly switch between configurations without having to edit the
config.yaml
file or rely on environment variables.Benefits
--target
flag, users can easily switch between LLM providers and models.The text was updated successfully, but these errors were encountered: