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

Add SambaNova Provider #54

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

snova-zoltanc
Copy link

No description provided.

@snova-zoltanc
Copy link
Author

I created a PR to add SambaNova as a provider, would appreciate help merging this! @rohitprasad15 @ksolo @standsleeping @andrewyng

Comment on lines 19 to 21
# NOTE: We could choose to remove above lines for api_key since OpenAI will automatically
# infer certain values from the environment variables.
# Eg: OPENAI_API_KEY, OPENAI_ORG_ID, OPENAI_PROJECT_ID, OPENAI_BASE_URL, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't apply in this case, can you remove the comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ksolo Thanks for review, I removed these comments.

Copy link
Collaborator

@rohitprasad15 rohitprasad15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except for the comment.
Please test it, and also add a block in examples/client.ipynb with sambanova provider and model.

"Sambanova API key is missing. Please provide it in the config or set the SAMBANOVA_API_KEY environment variable."
)

# Pass the entire config to the OpenAI client constructor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote in the comment, but I don't think you are passing the entire config.
Assuming that is what you want, you could add/replace the base_url in config and pass the entire thing to OpenAI client.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snova-zoltanc - please address above. We can merge it after that. Thank you.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitprasad15 Thanks for the review I made the update

@snova-zoltanc
Copy link
Author

@ksolo @rohitprasad15 I have addressed review, let me know if I can help with anything else before merging! 🙇

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

Successfully merging this pull request may close these issues.

3 participants