-
Notifications
You must be signed in to change notification settings - Fork 22
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
Google toolkit: Search and create contacts POC #249
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
str, | ||
"The search query for filtering contacts.", | ||
], | ||
page_size: Annotated[int, "The maximum number of contacts to return"] = 10, |
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.
thinking now, perhaps we should standardize arguments that are common across toolkits, regardless of how each 3pty API calls it.. in this case, maybe limit
would be an appropriate name?
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.
Absolutely 💯
|
||
# Warm-up the cache before performing search. | ||
# TODO: Ideally we should warmup only if this user (or google domain?) hasn't warmed up recently | ||
_warmup_cache(service) |
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.
Perhaps the warm-up could be implemented as a decorator, which could be aware of the last warm-up time and skip it, in case the index is already warm? not sure if this could be implemented easily considering our cloud engine has to support multi-tenancy... in a single tenant implementation, I imagine even an env var GOOGLE_CONTACTS_INDEX_LAST_WARMUP_TIMESTAMP
might suffice
|
||
|
||
@tool(requires_auth=Google(scopes=["https://www.googleapis.com/auth/contacts"])) | ||
async def create_contact( |
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.
does it make sense to create a contact that only has a name? in my mind, it should have at least a phone number, address or email address...
Marking this as ready for review because: Merging this does mean that we need to clean up/finish the tools before our next official Google toolkit release. |
This is a real thing: https://developers.google.com/people/v1/contacts#search_the_users_contacts | ||
""" | ||
service.people().searchContacts(query="", pageSize=1, readMask="names,emailAddresses").execute() | ||
await asyncio.sleep(3) # TODO experiment with this value |
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 does this function accomplish?
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.
Fixes the Google API being dumb. It is explained in the linked comment: https://developers.google.com/people/v1/contacts#search_the_users_contacts
return {"contacts": primary_results} | ||
|
||
|
||
async def _warmup_cache(service) -> None: # type: ignore[no-untyped-def] |
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.
type this?
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.
Open to suggestions here; the Google SDK is not typed which made it difficult to type the service
arg here.
email: Annotated[Optional[str], "The email address of the contact"], | ||
) -> Annotated[dict, "A dictionary containing the details of the created contact"]: | ||
""" | ||
Create a new contact in the user's Google Contacts using the People API. |
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.
"""Create a new contact record in Google Contacts.
This uses the Google People API to create and populate a new record with contact information
"""
Something like this might perform better. it's good to be concise so as to remove the possibility of a tool selection mismatch for a user query like "create a new user for the following people" which means to go hit posthog or something, but semantically seems to match well to the description. Eval this and try it out. be adversarial
@Spartee Note that I am intentionally excluding tests and evals in this PR because these tools are unfinished. I just need to get them to staging so that we can prove to Google that we are using the If we do not want the |
Merging work in progress. Filed Finish Google Contacts toolkit to finish it up. |
This is an initial sketch of what Contacts (People) API tools could look like. But I haven't yet thought like an MX Engineer @byrro 😉