feat: AI Tools & GenAI Functions - #622
Conversation
product-auto-label
Bot
added
the
api: alloydb
|
/gcbrun |
|
Is it possible to keep these PRs small? Helps to review them more quickly |
| async def _arun( | ||
| self, | ||
| content: str, | ||
| run_manager: Optional[CallbackManagerForToolRun] = None, |
There was a problem hiding this comment.
Seems like an unused arg. Present in multiple places. Please clean this up
| return str(result.scalar()) | ||
|
|
||
|
|
||
| class AlloyDBIfInput(BaseModel): |
There was a problem hiding this comment.
Should this include a user input? Same question for the other tools
| rows = result.fetchall() | ||
|
|
||
| compressed_docs = [] | ||
| # Fallback to standard 1-to-1 if returns raw scores array |
There was a problem hiding this comment.
Is the ambigous output type the official behaviour of the API?
| doc_idx = int(row[0]) - 1 # Postgres arrays are 1-indexed | ||
| doc = documents[doc_idx] | ||
| doc.metadata["relevance_score"] = float(row[1]) | ||
| except (ValueError, TypeError, IndexError): |
There was a problem hiding this comment.
What kind of cases are we taking here? Why are not changing the index here? Could you please clarify the use cases you're considering here?
| # Assuming (index, score) or (id, score) | ||
| # We will just map it positionally for now or try to extract index | ||
| try: | ||
| doc_idx = int(row[0]) - 1 # Postgres arrays are 1-indexed |
There was a problem hiding this comment.
For better clarity can we try using the RowMapping method? It allows us to fetch data using column names instead of numbers.
Eg, rows[0][0] might become rows[0]["id"]
| # It returned a single row with an array of scores | ||
| scores = rows[0][0] | ||
| for idx, score in enumerate(scores): | ||
| if self.top_n and idx >= self.top_n: |
There was a problem hiding this comment.
The scores logic always fails
| If the scores array is... | Line 88 (idx >= top_n) |
Line 90 (documents[idx]) |
|---|---|---|
| in input order | wrong — drops by position | right |
| pre-sorted by score | right — first n are the top n | wrong — pairs scores with unrelated docs |
| compressed_docs.sort(key=lambda x: x.metadata["relevance_score"], reverse=True) | ||
| if self.top_n: | ||
| compressed_docs = compressed_docs[:self.top_n] | ||
| else: |
There was a problem hiding this comment.
Why is there no top n sorting on this branch?
twishabansal
left a comment
twishabansal
left a comment
There was a problem hiding this comment.
Please fix lint issues
| @@ -0,0 +1,7 @@ | |||
| Document Compressor | |||
There was a problem hiding this comment.
Do the docs render correctly? Do they need to be added to the index: https://github.com/googleapis/langchain-google-alloydb-pg-python/blob/main/docs/index.rst?
| if self.top_n and idx >= self.top_n: | ||
| continue | ||
| doc = documents[idx] | ||
| doc.metadata["relevance_score"] = float(score) |
There was a problem hiding this comment.
Should we deep copy here to avoid collisions? (Needs update in multiple places in this method)
3 participants
Part 2 of 3 for AlloyDB AI Features Epic. Contains AlloyDB Document Compressor, Summary Tool, Sentiment Tool, and If Tool.