[fix][llm] preserve ToolCall.Extra when converting DO tool calls to eino schema - #631
Open
karthikchundi-commits wants to merge 1 commit into
Conversation
…ino schema FromDOToolCall (entity.ToolCall -> eino schema.ToolCall) dropped the Extra field, even though both types define an identical map[string]any Extra field, and the reverse conversion (ToDOToolCall) already copies it correctly. FromDOMessages/FromDOMessage - which call FromDOToolCall via FromDOToolCalls - are used directly as the outbound message input to chatModel.Generate/Stream in llmimpl/eino/llm.go, i.e. this is the conversion applied to stored conversation history before it's sent back to the model. Any provider-specific metadata a model attached to a prior tool call (stored in ToolCall.Extra by ToDOToolCall when the response first came in) was silently lost on every subsequent turn of a multi-turn tool-calling conversation. Added a regression test case with a populated Extra map; the existing test case only covered the nil case, which is why the asymmetry wasn't caught. I don't have a local Go toolchain in this environment, so I could not run `go test` or `go vet` - verified by reading the current eino schema.ToolCall definition directly from cloudwego/eino's source (schema/message.go) to confirm the Extra field exists with a matching map[string]any type on both sides, rather than assuming. Assisted-by: AI Signed-off-by: Karth <karthik.chundi@gmail.com>
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
FromDOToolCall(entity.ToolCall-> einoschema.ToolCall, inbackend/modules/llm/domain/entity/eino_convertor.go) dropped theExtrafield. Both types define an identicalmap[string]anyExtrafield, and the reverse conversionToDOToolCallalready copies it correctly - this was a one-directional gap, not an intentional omission.Why it matters
FromDOMessages/FromDOMessage(which callFromDOToolCallviaFromDOToolCalls) are used directly as the outbound message input tochatModel.Generate/Streaminbackend/modules/llm/domain/service/llmimpl/eino/llm.go- i.e. this conversion is applied to stored conversation history right before it's sent back to the model. Any provider-specific metadata a model attached to a prior tool call (stored inToolCall.ExtrabyToDOToolCallwhen that response first came in) was silently lost on every subsequent turn of a multi-turn tool-calling conversation.Fix
Add the missing
Extra: t.Extraassignment - a direct one-line mirror of whatToDOToolCallalready does in the other direction.Testing
Added a regression test case (
TestFromDOToolCalls) with a populatedExtramap, and extended the test's assertions to actually checkExtraequality - the existing test case only covered the nil case (Extra: nilon both sides), which is exactly why this asymmetry wasn't caught before.I don't have a local Go toolchain available in this environment, so I could not run
go test/go vetmyself. I verified the fix by reading the currentcloudwego/einosource directly (schema/message.go) to confirmschema.ToolCall.Extraexists with a matchingmap[string]anytype on both sides, rather than assuming. Happy to address any CI feedback.Checklist
go testlocally (not possible in this environment - see note above)