-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
core: allow artifact in create_retriever_tool #28903
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
eef8b40
to
c04c85d
Compare
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.
2 questions/suggestions, and will need some unit tests to be mergable
[await aformat_document(doc, document_prompt) for doc in docs] | ||
) | ||
|
||
if response_format == "content_and_artifact": | ||
return (content, [doc.model_dump() for doc in docs]) |
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.
might make sense to just return docs
as the artifact - any reason to dump here in particular?
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 reason is to always have the same type after deserialization.
As artifact
has type Any
when deserializing a ToolMessage
from json or any other source (api call, DB, etc), Pydantic won't be able to do any "magic" to regenerate the artifact as a list[Document]
but it will be a plain list[dict]
.
So, to be consistent and always have a dict I opted to dump.
Otherwise you need to add custom logic when deserializing to have consistent types.
format_document(doc, document_prompt) for doc in docs | ||
) | ||
if response_format == "content_and_artifact": | ||
return (content, [doc.model_dump() for doc in docs]) |
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.
same q as on async one
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.
Same reasoning as above
@@ -55,6 +66,7 @@ def create_retriever_tool( | |||
*, | |||
document_prompt: Optional[BasePromptTemplate] = None, | |||
document_separator: str = "\n\n", | |||
response_format: Literal["content", "content_and_artifact"] = "content", |
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.
needs to be added to docstring
Add option to return content and artifacts, to also be able to access the full info of the retrieved documents.
They are returned as a list of dicts in the
artifacts
property if parameterresponse_format
is set to"content_and_artifact"
.Defaults to
"content"
to keep current behavior.