-
Notifications
You must be signed in to change notification settings - Fork 116
Add Gemini Embeddings #1345
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
Add Gemini Embeddings #1345
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
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.
Main question was about the contents
value and I had one suggestion about a refactor but totally don't have to do that in this PR.
newrelic/hooks/mlmodel_gemini.py
Outdated
trace_id = linking_metadata.get("trace.id") | ||
try: | ||
embedding_content = kwargs.get("contents") | ||
embedding_content = str(embedding_content) if embedding_content else "" |
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.
The if embedding_content else ""
seems weird to me. Is this allowed to be a non string value? If it is None, then we are passing an empty string to the token count so we might as well not even call the token count function. Also it being None-is that option even allowed? How can you run an embedding without the input? Maybe this is just a leftover from before we added logic to not send value of type None and it's not needed anymore?
embedding_content = str(embedding_content) if embedding_content else "" | |
embedding_content = str(embedding_content) if embedding_content else "" |
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, it seems that you can pass a single string or a list of strings (ex: https://github.com/googleapis/python-genai/blob/main/google/genai/models.py#L4028). My thought was to typecast to string in case it is a list so that we send the full input.
In terms of embedding_content being None
, I was thinking more along the lines of there being a failure to grab the input from kwargs in the previous line:
embedding_content = kwargs.get("contents")
I was thinking down the road if they change the structure or argument names within kwargs
then we may not get anything back if we try searching for contents
within the request so this was to safeguard against that case.
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.
Ok that makes sense. In cases like that where the argument name might change, I think it's good to consider if it did-how would we catch it? Is this covering up that issue or exposing it so we can catch it quickly and fix it? Also, if this might be a list-how does that impact the token counting (might be something worth asking the AI team)? Would it be better to expand our spec for the token counting function to accept a list and cast it to a string after when recording the input into the event instead?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-gemini-instrumentation #1345 +/- ##
=================================================================
Coverage ? 81.46%
=================================================================
Files ? 205
Lines ? 22933
Branches ? 3629
=================================================================
Hits ? 18683
Misses ? 3033
Partials ? 1217 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Had one minor fixup and some continued discussion on the contents parameter. If you want to merge this and then investigate that further with the AI team I think that's fine since it's going to a feature branch anyway.
newrelic/hooks/mlmodel_gemini.py
Outdated
trace_id = linking_metadata.get("trace.id") | ||
try: | ||
embedding_content = kwargs.get("contents") | ||
embedding_content = str(embedding_content) if embedding_content else "" |
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.
Ok that makes sense. In cases like that where the argument name might change, I think it's good to consider if it did-how would we catch it? Is this covering up that issue or exposing it so we can catch it quickly and fix it? Also, if this might be a list-how does that impact the token counting (might be something worth asking the AI team)? Would it be better to expand our spec for the token counting function to accept a list and cast it to a string after when recording the input into the event instead?
This PR adds instrumentation + tests for sync and async embeddings in
google-genai
.