feat(core): output_version on invoke / model call
#32694
Conversation
This reverts commit 0ddab9f.
CodSpeed Instrumentation Performance ReportMerging #32694 will not alter performanceComparing Summary
|
mdrxy
changed the title
output_version on invoke / mode lcalloutput_version on invoke / model call
…gchain into mdrxy/call-version
| from langchain_core.output_parsers.openai_tools import ( | ||
| JsonOutputKeyToolsParser, | ||
| PydanticToolsParser, | ||
| ) |
There was a problem hiding this comment.
Did we dig into why this was here? I'm assuming it was to resolve a circular import.
There was a problem hiding this comment.
Possibly, or just an erroneous placement (since we don't enforce placement with ruff). Moving didn't break anything 🙏
| effective_output_version = ( | ||
| output_version if output_version is not None else self.output_version | ||
| ) | ||
| kwargs["_output_version"] = effective_output_version or "v0" | ||
|
|
||
| # Whether the user explicitly set an output_version for either model or call | ||
| kwargs["_output_version_explicit"] = ( | ||
| output_version is not None or self.output_version is not None | ||
| ) |
There was a problem hiding this comment.
instead of determining this here, can we just propagate
kwargs["_output_version"] = output_versionand then at call sites, do
output_version = kwargs.pop("_output_version", None) or self.output_version
message = _update_message_content_to_blocks(message, output_version)then update _update_message_content_to_blocks (maybe rename to process_output_version or similar) to do something like
def _update_message_content_to_blocks(message: T, output_version: str) -> T:
if output_version is None:
return message
elif output_version == "v1":
return message.model_copy(
update={
"content": message.content_blocks,
"response_metadata": {
**message.response_metadata,
"output_version": output_version,
},
}
)
else:
# v0
return message.model_copy(
update={
"response_metadata": {
**message.response_metadata,
"output_version": output_version,
},
}
)| filtered_kwargs = { | ||
| k: v | ||
| for k, v in kwargs.items() | ||
| if k not in ("_output_version", "_output_version_explicit") | ||
| } |
There was a problem hiding this comment.
(nit) see above comment on just popping _output_version from kwargs and processing
| "'output_version' should have the same value across " | ||
| "all chunks in a generation." | ||
| ) | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
BaseChatModel is inserting the output version, right? how would we hit this ValueError?
If a user receives this ValueError, are they just stuck? What can they do?
I think in most other cases in the merging logic we just accept the left value.
| from langchain_core.outputs import ChatGeneration, ChatGenerationChunk, ChatResult | ||
|
|
||
|
|
||
| class OutputVersionTrackingChatModel(GenericFakeChatModel): |
There was a problem hiding this comment.
can we not use GenericFakeChatModel on its own?
| "[('_output_version', 'v0'), ('_output_version_explicit', False), ('_type'," | ||
| " 'generic-fake-chat-model'), ('stop', None)]", |
| # Note: output_version accepted for interface consistency; format conversion | ||
| # handled by core |
There was a problem hiding this comment.
are we introducing type errors into all chat models?
could we just add to signature of top-level .invoke, .stream, etc. instead of _generate, _stream and propagate through kwargs?
| elif output_version == "v1": | ||
| # Use content_blocks property which handles v1 conversion via block_translators | ||
| message = message.model_copy(update={"content": message.content_blocks}) |
There was a problem hiding this comment.
isn't this done entirely in core? we went to some lengths so that we wouldn't have to touch the chat model implementations.
| effective_output_version = ( | ||
| output_version | ||
| if output_version is not None | ||
| else (self.output_version or "v0") | ||
| ) |
There was a problem hiding this comment.
is this logic needed? ideally we should need zero or minimal changes to child chat model classes.
Q: If
output_versionis not set,response_metadatawon't have anoutput_version, even though it defaults to'v0'. Do we want this?