-
Notifications
You must be signed in to change notification settings - Fork 94
Add embed
to Index configure
calls
#515
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
…igure method, and building in the request factory
…how we're building the configure index request call to be a bit more flexible
…s ConfigureIndexRequestEmbed as the class to the generated core
… interface and asyncio interface to support the embed parameter on configure
@@ -98,6 +98,11 @@ | |||
"RestoreJobList": ("pinecone.db_control.models", "RestoreJobList"), | |||
"BackupModel": ("pinecone.db_control.models", "BackupModel"), | |||
"BackupList": ("pinecone.db_control.models", "BackupList"), | |||
"ConfigureIndexEmbed": ("pinecone.db_control.types", "ConfigureIndexEmbed"), |
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.
what's the reason for adding ConfigureIndexEmbed and CreateIndexForModelEmbedTypedDict here?
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.
Adding the classes to _db_control_lazy_imports
, which we seem to do for all of our custom models and types. It seemed like CreateIndexForModelEmbedTypedDict
was also not included here, so I added it.
This is to allow lazy loaded imports to be exported from the top level of the package here:
pinecone-python-client/pinecone/__init__.py
Line 145 in dfd0125
*list(_LAZY_IMPORTS.keys()), |
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.
yeah I was wondering why we added types here but here's the reason why:
#507 (comment)
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.
LGTM! Nice work. Also it makes sense to follow CreateIndexForModelEmbed
approach.
Problem
embed
was never exposed as an argument for callingconfigure
onIndexResource
.Solution
ConfigureIndexEmbed(TypedDict)
class for representing the argument dictionary shape. I went with this because it aligned with the existingCreateIndexForModelEmbedTypedDict
, but I'm not sure if this is best practice in the repo at this point. Maybe a class would be better.embed
onconfigure
calls.Pinecone.configure_index
method to supportembed
.configure
orconfigure_index
.Type of Change
Test Plan
New integration tests added. You can pull this branch down and use poetry to run a repl and quickly evaluate things yourself: