nvms / wingman

Your pair programming wingman. Supports OpenAI, Anthropic, or any LLM on your local inference server.
https://marketplace.visualstudio.com/items?itemName=nvms.ai-wingman
ISC License
61 stars 10 forks source link

Use SecretStorage #5

Closed nvms closed 1 year ago

nvms commented 1 year ago

RE: #4

One thing to consider is that when using LLaMa for generation, which uses the OpenAI API, the user will be prompted for an OpenAI API key and they may not have one. Still undecided on how best to handle this.

Do we need to expose a way via the UI to destroy these secrets?

capdevc commented 1 year ago

I think there should be a command to trigger manually for setting API tokens, maybe with a dropbox for provider selection. With that I don't think deletion is strictly necessary. You could just overwrite the secret with an empty string or whatever you like.

Re: locally hosted, you could only use the API key when the apiURL isn't the default one. For Anthropic this is hard coded in the SDK. I think the way you have it the key is only prompted for at time of use, so that would solve that problem. Is there a scenario where you want an openai api key to use with a non-openai API endpoint?

You could also decouple the API from the provider

"wingman.providers": [
  {  
    "name": "openai",
    "apiType": "openai",
    "apiKeySecret": "openai",
  },
  {  
    "name": "localLLaMa",
    "apiType": "openai",
    "apiUrl": "https://localhost/...",
  },
  {  
    "name": "anthropic",
    "apiType": "anthropic",
    "apiKeySecret": "anthropic",
  },
]

Here the provider gets looked up by name (given in the command definition), the apiUrl (optional) overrides the default URL for that API, and the apiKeySecret is the name of the secret for that provider, and no API key is needed if no secret is named.

This may be a little complicated, but defaults could be added for anthropic and openai so you'd only need to interact with it if you wanted to add a non-standard provider for a given API. Presumably if you want to do that you could figure this out.

nvms commented 1 year ago

I think there should be a command to trigger manually for setting API tokens, maybe with a dropbox for provider selection. With that I don't think deletion is strictly necessary. You could just overwrite the secret with an empty string or whatever you like.

I like this idea a lot and implemented it here: 7b6160e54bf12f712bc2765c13367ff7132f631f

Is there a scenario where you want an openai api key to use with a non-openai API endpoint?

No, I don't think so

A thought: when you have an invalid OpenAI API key, you will see this warning:

Screenshot 2023-06-11 at 9 17 57 PM

Since creating a ChatGPTAPI requires an API key to be provided (https://github.com/transitive-bullshit/chatgpt-api/blob/main/src/chatgpt-api.ts#L102-L104), I wonder if it'd be reasonable to just assign a default value of, e.g. "llama". This means that the local llama solution would "just work" without any extra configuration (other than changing wingman.openai.apiBaseUrl), and a person trying to use ChatGPT will see the above warning and be prompted to set/change their key.

But...

Decoupling the provider from the API style is a cleaner solution, though, and I'll likely take this PR in that direction. Another benefit of decoupling is that it allows for defining other third party solutions that might share an API style of either OpenAI or Anthropic.

capdevc commented 1 year ago

I'd suggest a FAQ or something for "How do I delete my token?" since overwriting it may not be obvious to a user.

The anthropic API also has error codes for bad tokens etc: https://docs.anthropic.com/claude/reference/errors-and-rate-limits so you could prompt on that specific response as well.

I do think that decoupling providers from the API is the path forward since it seems possible that a standard will evolve here.

nvms commented 1 year ago

I think the scope of this PR has been met. Probably best to save the decoupling provider stuff for another PR after giving it a bit more thought about how best to implement it.

Llama support remains, but it's not a great user experience and will be greatly improved by the decoupling as mentioned before.