Skip to content

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Sep 10, 2025

@cbornet cbornet requested a review from eyurtsev as a code owner September 10, 2025 16:40
Copy link

vercel bot commented Sep 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
langchain Ready Ready Preview Comment Sep 10, 2025 5:10pm

Copy link

codspeed-hq bot commented Sep 10, 2025

CodSpeed WallTime Performance Report

Merging #32888 will not alter performance

Comparing cbornet:merge_dict (60c6521) with wip-v1.0 (544b08d)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

✅ 13 untouched benchmarks

@cbornet
Copy link
Collaborator Author

cbornet commented Sep 10, 2025

cc @baskaryan as you introduced the TODO.

@cbornet cbornet requested review from baskaryan and removed request for eyurtsev September 10, 2025 16:45
@cbornet cbornet changed the title feat(core)!: raise exception if 'type' value is different in dicts to merge feat(core)!: raise exception if type value is different in dicts to merge Sep 10, 2025
Copy link

codspeed-hq bot commented Sep 10, 2025

CodSpeed Instrumentation Performance Report

Merging #32888 will not alter performance

Comparing cbornet:merge_dict (60c6521) with wip-v1.0 (544b08d)

Summary

✅ 14 untouched benchmarks

@cbornet
Copy link
Collaborator Author

cbornet commented Sep 10, 2025

Some tests fail:

assert AIMessageChunk(
        content=[{"index": 0, "text": "I am", "type": "text_block"}]
    ) + AIMessageChunk(
        content=[{"index": 0, "text": " indeed.", "type": "text_block_delta"}]
    ) == AIMessageChunk(
        content=[{"index": 0, "text": "I am indeed.", "type": "text_block"}]
    ), (
        "Concatenating when both content arrays are dicts with the same index "
        "and different types should merge without updating type"
    )

It seems the merge_dicts should now allow to merge with different types ?

So should we:

  1. remove the TODOs and related code (basically revert core[patch]: dont mutate merged lists/dicts #25858) ?
  2. or modify the AIMessageChunk tests ?
  3. or have an optional param in merge_dicts to allow merging dicts with different types ?
  4. other ?

@mdrxy mdrxy added the core Related to the package `langchain-core` label Sep 10, 2025
@mdrxy mdrxy added this to the v1 milestone Sep 10, 2025
@mdrxy mdrxy mentioned this pull request Sep 10, 2025
18 tasks
@cbornet
Copy link
Collaborator Author

cbornet commented Sep 15, 2025

So should we:
remove the TODOs and related code (basically revert #25858) ?
or modify the AIMessageChunk tests ?
or have an optional param in merge_dicts to allow merging dicts with different types ?
other ?

@eyurtsev @mdrxy can I have your opinion on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to the package `langchain-core`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants