Skip to content
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

vertexai[patch]: remove allOf from function schema #96

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

baskaryan
Copy link
Contributor

patch for #95

@baskaryan
Copy link
Contributor Author

@alx13 @lkuligin

if isinstance(v, dict) and "allOf" in v:
if isinstance(v["allOf"], list) and len(v["allOf"]) == 1:
obj = dict(v["allOf"][0])
obj["title"] = v.get("title", obj.get("title", ""))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

title field is now not supported - we are dropping it

obj = dict(v["allOf"][0])
obj["title"] = v.get("title", obj.get("title", ""))
obj["description"] = " ".join(
(v.get("description", ""), obj.get("description", ""))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about joining descriptions.

This code:

class Node(BaseModel):
    """This is a node"""
    id: str
    type: str

class Relationship(BaseModel):
    source: Node
    target: Node = Field(..., description="This is a target node")

will produce following definition for target property:

{
  "target": {
    "title": "Target",
    "description": "This is a target node This is a node",
    "type": "object",
    "properties": {
      "id": {
        "title": "Id",
        "type": "string"
      },
      "type": {
        "title": "Type",
        "type": "string"
      }
    },
    "required": ["id", "type"]
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea not ideal but is there a better generic solution? both descriptions could have important (and distinct) info

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider coalescing i. e. field description should take precedence

class Relationship(BaseModel):
    source: Node
    target: Node = Field(..., description="This is a target node")

over model description:

class Node(BaseModel):
    """This is a node"""
    id: str
    type: str

And could you please describe reasoning and behaviour in the function doc string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can get behind that, will update!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lkuligin what is your take on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants