Add VertexAI provider #11
Conversation
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Calvin Giles <c***@t***.c***.nz>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated. |
calvingiles
force-pushed
the
add-vertex-provider
branch
from
a284ba4
to
8470f1c
Compare
|
Thanks for the contribution! Before we can merge this, we need @calvingiles to sign the Salesforce Inc. Contributor License Agreement. |
There was a problem hiding this comment.
@calvingiles thanks a ton for this contribution! It's super neat to see the diff of a new provider! 🤖 ✨
This is all looking solid to me though I did leave a comment around environment variables and documentation that'd make setup a bit easier 🙏 Otherwise, thanks for tidying up a few places elsewhere!
I'll want to check with the team that we're equipped to maintain additional providers (and do a bit more testing myself) but I think this is off to a great start. Thanks again for sending it in and signing the CLA 🚀
|
I have made the changes according to your comments and done a general pass of the README for OpenAI and Anthropic comments to mention Vertex too to avoid confusion. |
|
I am happy to make any other changes to fit in with the project. |
There was a problem hiding this comment.
This is super great! Thanks so much for those updates 🙏
I gave the models a run and am always impressed with the speed of response, but I did find one error with a model that we might want to address. I left a few thoughts, but feel free to suggest what you find best!
The README was also reading well and I took a second pass at it with some of the confusions I was finding during setup 📚 I also left a few nits in comments that might not be so important, so feel free to ignore these.
After things are looking good to you, I think we're in a good place to merge this! The Gemini models are widely used and battle-tested, and I'm hoping others will find it neat to find here. Exciting!! 🚀
|
Thanks! The comments all seem reasonable so will make the required fixes. |
…hon-ai-chatbot into add-vertex-provider
|
All changes complete - I think we are ready to merge? |
There was a problem hiding this comment.
This is looking solid to me! Really appreciate the updates 🏆
I made a few more changes that were mostly formatting related (sorting imports and similar) but also updated the system_instruction setup slightly. I'm a fan of adding details to the models, but now set the client settings as unique variables before creating the client.
I left a few notes on this and am interested in how this might continue to change with updating models, but for now I think we're set to ship 🚢 💨
All of the models work will with this setup and it's neat to switch between all of the selections 👀
| "name": "Gemini 1.0 Pro 001", | ||
| "provider": VERTEX_AI_PROVIDER, | ||
| "max_tokens": 8192, | ||
| "system_instruction_supported": False, |
There was a problem hiding this comment.
This is a super nice approach and I'm glad it can be known within the models of this class 🙏
| system_instruction = None | ||
| if self.MODELS[self.current_model]["system_instruction_supported"]: | ||
| system_instruction = system_content | ||
| else: | ||
| prompt = system_content + "\n" + prompt |
There was a problem hiding this comment.
Updated this to remove **kwargs since it wasn't so clear which arguments were being updated before generation and I was finding this error with pyright:
Argument of type "str" cannot be assigned to parameter "tool_config" of type "ToolConfig | None" in function "__init__"
Type "str" is not assignable to type "ToolConfig | None"
"str" is not assignable to "ToolConfig"
"str" is not assignable to "None"I found that these changes continue to work fine for both cases, and I'm hoping the set variables before model setup helps make future updates clear 🔭
|
Thanks! Those changes all make sense. As an aside, I was reading the docs again and noticed that they replicate some of the instructions here - you might want to add some of the additions from the readme there too? Not sure where they get populated from though. |
|
@calvingiles Great call! That page is saved in the slackapi/bolt-python repo as part of the docs here 🔍 We definitely want to make similar updates to that page to keep things fresh, and we should also make a few notes on this in a All of your changes already have been great, so don't feel obligated to tackle this too! I'll plan to take a pass at these updates in the next few days 🙏 |
Type of change (place an x in the [ ] that applies)
Summary
Adding VertexAI as a provider (using gcloud credentials).
I have tested this works with a development slack app.
Requirements (place an x in each [ ] that applies)