-
Notifications
You must be signed in to change notification settings - Fork 21
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
update to 0.1, remove deprecated functionality and focus on api catalog backend #48
Conversation
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.
@mattf mostly reviewed the tests, docs, and notebook changes. All lgtm! Thanks
infer_path = "{base_url}/embeddings" | ||
# not all embedding models are on https://integrate.api.nvidia.com/v1, | ||
# those that are not are served from their own endpoints | ||
if model := determine_model(self.model): |
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 do see code for validating model determine_model in _NVIDIAClient, is this necessary to determine again? As determine_model returns Model obj, we can check the endpoint is present via _client.model.endpoint.
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.
unfortunately yes. the self.model is only the model's id str. this call gets the full Model so the endpoint can be checked.
a better approach is to store the Model instead of the id str.
self.model = self._client.model | ||
|
||
# todo: remove when nvolveqa_40k is removed from MODEL_TABLE | ||
if "model" in kwargs and kwargs["model"] in [ |
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.
Is this to validate the original model user input? why checking from kwargs?
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.
yes, checking the original model input by the user. at this point in the code self.model will be a transformed version of the original input, e.g. nvolveqa_40k -> NV-Embed-QA.
# those that are not are served from their own endpoints | ||
if model := determine_model(self.model): | ||
if model.endpoint: # some models have custom endpoints | ||
infer_path = model.endpoint |
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.
Same comment as embedding, can we check model.endpoint after the _NVIDIAClient initialization?
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.
you are quite correct. this is code that's duplicated across all connectors and should move into _NVIDIAClient.
i ran into an issue w/ differing types when making the _NVIDIAClient.model as Model instead of a str.
let me follow up this commit w/ cleanup of the Model/str handling.
…ves, remove those without an alternative
…oogle/codegemma-1.1-7b to set of ChatNVIDIA models
this is version 0.1.0 of the connectors with two primary changes -
all playground_* model endpoints have been decommissioned.
functionality removed -
functionality deprecated -
migration guide -
ChatNVIDIA().mode("nim", base_url="http://...")
must useChatNVIDIA(base_url="http://...")
NVIDIAEmbeddings().mode("nim", base_url="http://...")
must useNVIDIAEmbeddings(base_url="http://...")
NVIDIARerank().mode("nim", base_url="http://...")
must useNVIDIARerank(base_url="http://...")
model migration, the following models will raise a warning and use an alternative (model changes in italics) -