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

feat: support many:1 auth:provider mapping #929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VaibhavMalik4187
Copy link
Contributor

@VaibhavMalik4187 VaibhavMalik4187 commented Feb 8, 2024

📑 Description

Added the ability to add multiple configurations for the same backend
provider.

One to one auth:provider mapping had many bugs and users were requesting
many to one auth:provider mapping functionality for more flexibility
with backend provider configurations.

This commit also includes the addition of an optional --config-name
flag to allow users to specify the config-name while configuring backend
AI providers.

Changes exclusive to individual subcommands are:

  1. default
  • --config-name flag has been added to allow users to update the
    default configuration for a backend AI provider.
  • Setting the --config-name flag will only update the default
    configuration for the specified backend, it will not update the
    configAI.DefaultProvider.
  • To update the configAI.DefaultProvider, user must leave the
    --config-name flag unset.
  1. list
  • The list subcommand will now respect the value of userInput set
    when the user is asked to show password or not.
  • The command's output has been modified to display the backend
    providers list along with their configuration names.
  • Default configs for each backend AI provider are marked with the
    (Default Config) string in bright yellow color.
  1. remove
  • Users can specify either single or multiple backends when
    removing the configurations.
  • If the --config-name flag is not set, the "default" configuration
    for the specified backend(s) will be removed(if it is present).
  • Setting the --config-name flag will specify the configuration name
    to delete for each of the specified backends.
  1. Update
  • Now the user can only specify a single backend provider at a time.
    This has been done because updating multiple backend configs with
    same values of parameters doesn't make any sense.
  • Removal of the Args function because update subcommand can have
    multiple args to set different parameters for a backend config.
  • Users can also update the configuration name for an already existing
    config.

Addresses:

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

@VaibhavMalik4187 VaibhavMalik4187 requested review from a team as code owners February 8, 2024 08:26
@VaibhavMalik4187 VaibhavMalik4187 changed the title feat: CLI Backend overhaul feat: cli backend overhaul Feb 8, 2024
@VaibhavMalik4187
Copy link
Contributor Author

@AlexsJones @bwplotka I've added initial code to facilitate *:1 auth:provider mapping. In this version, the AI backend configuration is stored in the following format:

ai:
    providers:
        - backend: openai
          configs:
            - name: default
              model: gpt-3.5-turbo
              password: some-pass
              temperature: 0.7 
              topp: 0.5 
              maxtokens: 2048
            - name: New 
              model: gpt-3.5-turbo
              password: pass
              temperature: 0.7 
              topp: 0.5 
              maxtokens: 2048
          defaultconfig: 0
        - backend: localai
          configs:
            - name: New 
              model: gpt-3.5-turbo
              temperature: 0.7 
              topp: 0.5 
              maxtokens: 2048
          defaultconfig: 0
    defaultprovider: ""
kubeconfig: ""
kubecontext: ""

I plan to add another subcommand to the auth command to allow the user to set the default config name for each backend provider. Before I do that, I'd like to know if this is going in the right direction.

@VaibhavMalik4187
Copy link
Contributor Author

@AlexsJones I've updated the code to allow users to set the default config for an AI backend. However, I'm unsure how to proceed with the auth remove and auth update commands. These commands process a list of backends and, with many-to-one auth: provider mapping, we'll need to specify the config name as well.

How about the following approach:

  1. Take input single backend and single config-name. Delete the specified config from the specified backend.
  2. Provide a --delete-all-configs flag, allowing the user to delete multiple backends at a time.

@VaibhavMalik4187 VaibhavMalik4187 changed the title feat: cli backend overhaul feat: support many:1 auth:provider mapping Feb 11, 2024
@VaibhavMalik4187
Copy link
Contributor Author

@AlexsJones, this PR is ready for an initial review. Could you please take a look? Thanks in advance!

@VaibhavMalik4187
Copy link
Contributor Author

@matthisholleville, could you please go through this PR whenever you have a moment? Thanks!

@VaibhavMalik4187
Copy link
Contributor Author

@JuHyung-Son could you please take a look at this PR? Thanks!

@VaibhavMalik4187
Copy link
Contributor Author

@AlexsJones ...

@AlexsJones
Copy link
Member

Apologies, I missed this one @VaibhavMalik4187

Can you show me how you'd use a different configuration for a backend?
The commands aren't clear to me

@VaibhavMalik4187 VaibhavMalik4187 force-pushed the cli-backend-overhaul branch 2 times, most recently from 3e12bbb to 7c5ad16 Compare April 30, 2024 13:51
@VaibhavMalik4187
Copy link
Contributor Author

VaibhavMalik4187 commented Apr 30, 2024

Apologies, I missed this one @VaibhavMalik4187

Can you show me how you'd use a different configuration for a backend? The commands aren't clear to me

No problem @AlexsJones. Sure, here we go:

  1. Listing the backend configurations.
    image

  2. Adding a new backend without using the config-name flag.
    image

  3. Adding a new backend with the config-name flag specified.
    image

  4. Changing the default backend without specifying the config-name
    image

  5. Changing the default config for an ai provider
    image

  6. Running analysis by specifying different configs for the same backend.
    image

I know that this is a very large change and hope these examples make things slightly easier to understand.

Added the ability to add multiple configurations for the same backend
provider.

One to one auth:provider mapping had many bugs and users were requesting
many to one auth:provider mapping functionality for more flexibility
with backend provider configurations.

This commit also includes the addition of an optional `--config-name`
flag to allow users to specify the config-name while configuring backend
AI providers.

Changes exclusive to individual subcommands are:
1. default
  - `--config-name` flag has been added to allow users to update the
    default configuration for a backend AI provider.
  - Setting the `--config-name` flag will only update the default
    configuration for the specified backend, it will not update the
    `configAI.DefaultProvider`.
  - To update the `configAI.DefaultProvider`, user must leave the
    `--config-name` flag unset.

2. list
  - The list subcommand will now respect the value of `userInput` set
    when the user is asked to show password or not.
  - The command's output has been modified to display the backend
    providers list along with their configuration names.
  - Default configs for each backend AI provider are marked with the
    `(Default Config)` string in bright yellow color.

3. remove
  - Users can specify either single or multiple backends when
    removing the configurations.
  - If the `--config-name` flag is not set, the "default" configuration
    for the specified backend(s) will be removed(if it is present).
  - Setting the `--config-name` flag will specify the configuration name
    to delete for each of the specified backends.

4. Update
  - Now the user can only specify a single backend provider at a time.
    This has been done because updating multiple backend configs with
    same values of parameters doesn't make any sense.
  - Removal of the `Args` function because update subcommand can have
    multiple args to set different parameters for a backend config.
  - Users can also update the configuration name for an already existing
    config.

Addresses:
- k8sgpt-ai#936
- k8sgpt-ai#911
- k8sgpt-ai#905
- k8sgpt-ai#900
- k8sgpt-ai#843

Signed-off-by: VaibhavMalik4187 <vaibhavmalik2018@gmail.com>
@JuHyung-Son
Copy link
Contributor

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

@VaibhavMalik4187
Copy link
Contributor Author

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

If we agree to move forward with this change, we'll also need to update the operator accordingly.

@AlexsJones
Copy link
Member

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

If we agree to move forward with this change, we'll also need to update the operator accordingly.

Would we have to, or is this more about parity?

@VaibhavMalik4187
Copy link
Contributor Author

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

If we agree to move forward with this change, we'll also need to update the operator accordingly.

Would we have to, or is this more about parity?

I think it is necessary to include the ConfigName field in the AnalyzeRequest data structure (see https://github.com/k8sgpt-ai/k8sgpt/pull/929/files#diff-81b03b26debf92e0e0bcaa55bdfcfbfc72a5a9ebafa949bb80b2474e759f506a)

@JuHyung-Son
Copy link
Contributor

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

If we agree to move forward with this change, we'll also need to update the operator accordingly.

Would we have to, or is this more about parity?

I think it is necessary to include the ConfigName field in the AnalyzeRequest data structure (see https://github.com/k8sgpt-ai/k8sgpt/pull/929/files#diff-81b03b26debf92e0e0bcaa55bdfcfbfc72a5a9ebafa949bb80b2474e759f506a)

Im not that sure we need to provide multiple auth keys. I think it's unnecessarily complicated and hinders the user experience a lot, and the user can just replace it with a different config via the k8sgpt -c option.

@VaibhavMalik4187
Copy link
Contributor Author

I haven't tested it, but it looks good from the screenshots. �However I think we need to discuss if this change to enable multiple configs is really necessary.

If we agree to move forward with this change, we'll also need to update the operator accordingly.

Would we have to, or is this more about parity?

I think it is necessary to include the ConfigName field in the AnalyzeRequest data structure (see https://github.com/k8sgpt-ai/k8sgpt/pull/929/files#diff-81b03b26debf92e0e0bcaa55bdfcfbfc72a5a9ebafa949bb80b2474e759f506a)

Im not that sure we need to provide multiple auth keys. I think it's unnecessarily complicated and hinders the user experience a lot, and the user can just replace it with a different config via the k8sgpt -c option.

Hmm, fair enough. In my opinion, these changes won'thinder the user experience but provide more flexible approach to setting the AI configurations. The worst part is this PR also fixes a bunch of issues other than many:1 auth provider mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants