-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] Add Baseten integration #4189
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
thank you for the PR @philipkiely-baseten. though i don't believe this is complete. It has to implement call to allow users to invoke it. And if you want to allow js client users to use this embedding function, there also needs to be an implementation in the js client, as well as schema, and a couple other functions. Happy to iterate on this, but until this is complete i would recommend moving it to draft. This is not ready to be merged yet. |
@jairad26 this works as implemented as it inherits the OpenAI integration's Is a JS client implementation required for all integrations? |
ah missed that it implements OpenAI embedding function instead of the EmbeddingFunction, thanks for pointing that out. The JS client is not required. Will run the workflows |
Could you update the PR title, and the default env var to match other embedding functions? The format should be |
@jairad26 thank you! We're hoping to go live tomorrow with a joint GTM announcement of this integration, if there are any issues with the workflows I will update right away |
On it! |
@jairad26 done on both counts! |
@jairad26 do we need to add a |
yea just looked at the tests, youll need to add BasetenEmbeddingFunction to the list of expected_builtins in |
Thank you, added |
@jairad26 hoping to go live in a couple hours -- everything is approved on the content side w/ jeff just need to get this merged to unblock |
hey @philipkiely-baseten sorry was not aware of the launch, running the workflows. will prioritize this |
@jairad26 looking at the failing tests its something with a zip file -- is this related to my change? |
not related to you this was fixed yday. could you rebase off of main |
@jairad26 done -- used the sync fork feature on GitHub, lmk if it looks correct |
the bench flakes we can rerun. also to double check, you took a pass and tested the embedding function correct? ill also take a pass once these go through to double check |
@jairad26 yes, I tested the workflow that is documented in |
@jairad26 here it is in my notebook using a model deployed in my baseten acct ![]() |
lovely, will also set up and test this as well just for sanity check. Excited for your launch! |
LMK if you have any issues getting a model deployed on Baseten or getting the model ID |
looks like theres some errors on a couple tests.. looking |
theres a couple of changes necessary. i misspoke earlier when i said the other functions are not required. heres the full file, can you replace your baseten_embedding_function with this
|
and could you rerun the embedding function verify it works, i can get to it after lunch |
f9e1d8d
to
d9d9ce9
Compare
@jairad26 sorry, just messed up the history. I'm sitting on a flight about to take off. Will ping when fixed. |
d9d9ce9
to
5a3d8fb
Compare
@jairad26 Sorry for the weirdness on Git -- never using GitHub's "sync fork" feature again. Anyway, the PR should be in a good state now, using your version of the file with the extra functions! Thank you for that! |
you also need to add it to known_embedding functions |
Also, rebuilt the package locally and verified in notebook, worked as expected |
sorry missed that file, yep looks good! thanks for all the iteration |
@jairad26 thank you for your help! Fingers crossed there are no issues on the checks |
7318c7c
to
911697f
Compare
Description of changes
This PR adds a Baseten integration for using Chroma with models deployed on Baseten
Test plan
Tested locally with deployed model, output is identical to OpenAI integration.
Documentation Changes
Integration is documented in appropriate file.