-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ewiz sql #18
Ewiz sql #18
Conversation
elm/utilities/try_import.py
Outdated
|
||
Returns | ||
------- | ||
p : package |
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 function doesn't return anything
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.
Doesn't this return the import? p = importlib.import_module(package_name) return p
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.
oh sorry! it does. I missed the return statement nested in there. looks fine
|
||
body = json.dumps({"inputText": text, }) | ||
|
||
model_id = self.EMBEDDING_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.
Is this supposed to be the titan model name? Seems like this will still be 'text-embedding-ada-002'
based on the ApiBase
class. I think you need to set the class variable for this class.
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.
@grantbuster When I create the wizard object in the run_app.py code I set the model_id using EnergyWizardPostgres.EMBEDDING_MODEL = 'amazon.titan-embed-text-v1'
and that seems to do the trick. I was thinking doing it that way would be most consistent with the existing code, would it be better to set the class variable?
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 we should set the class variable because in the get embedding method we hard-code the call to the AWS object, right?
In the docstring we may want to have a description of how this is designed, e.g.:
vector database - postgresql with psycopg2
embedding model - aws titan
llm - GPT4 via Azure deployment
@spodgorny9 can you also add a test for the new class? Please mock the postgres responses. |
tests/test_postgres.py
Outdated
|
||
@staticmethod | ||
def ref_call(*args, **kwargs): # pylint: disable=unused-argument | ||
"""Mock for EnergyWizardPostgres.make_ref_list()""" |
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 is fine but i was hoping you would mock the psycopg2/boto3 objects and not our methods. Ideally you want to preserve as much of our code as possible and only mock the functions in the dependencies that we cannot call in the test env
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, thanks for working through this with me.
Adding code to access postgres vector database and generate query embeddings from AWS. Also includes try_import utility.