-
Notifications
You must be signed in to change notification settings - Fork 36
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 NVIDIA as a Provider and expose it to Goose #132
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Praateek <praateekm@gmail.com>
Signed-off-by: Praateek <praateekm@gmail.com>
Signed-off-by: Praateek <praateekm@gmail.com>
Signed-off-by: Praateek <praateekm@gmail.com>
@@ -19,6 +19,8 @@ packages = [{ include = "goose", from = "src" }] | |||
[tool.hatch.build.targets.wheel] | |||
packages = ["src/goose"] | |||
|
|||
|
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.
Was this an accidental change?
src/goose/cli/config.py
Outdated
} | ||
processor, accelerator = recommended.get(provider, ("gpt-4o", "gpt-4o-mini")) | ||
processor, accelerator = recommended.get(provider, recommended["nvidia"]) |
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 think this line should be reverted. Is there a reason to change the default values to nvidia's values?
Your change on line 105 should work to set the default configuration to nvidia when it detects NVIDIA_API_KEY.
self.client = client | ||
|
||
@classmethod | ||
def from_env(cls: Type["NVIDIAProvider"]) -> "NVIDIAProvider": |
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 works! however @elenazherdeva recently introduced cls.check_env_vars()
pattern so these can be checked upfront so we can avoid the need for a try/except
block
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.
looks great - if can just get that small tweak
@praateekmahajan you ok if I take this on and finish it up to get it in? |
@michaelneale hey sorry for the late response, I had marked it as draft since it was a POC, and I was trying to get it to work with LLAMA 3.1 but wasn't able to. If you've been able to that's great. And thanks for taking it over, I haven't had the bandwidth to look at this and make it work yet. |
@praateekmahajan no worries - yes at some point we will iterate a bit on the newer llamas (main thing was tool calling was not great generally yet - but will get better) |
Creates a provider in
exchange
.In goose, adds a config for the NVIDIA provider (using
llama-3.1-405b
) todefault_model_configuration