Skip to content

Python: fix(checkpoint): preserve dicts whose keys collide after str() - #8272

Merged
Evan Mattson (moonbox3) merged 4 commits into
microsoft:mainfrom
ktz03:fix/checkpoint-dict-key-collision
Sep 15, 2026
Merged

Python: fix(checkpoint): preserve dicts whose keys collide after str()#8272
Evan Mattson (moonbox3) merged 4 commits into
microsoft:mainfrom
ktz03:fix/checkpoint-dict-key-collision

Conversation

@ktz03

@ktz03 LI (ktz03) commented Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation & Context

Checkpoint encoding stringifies dict keys for JSON. Distinct Python keys that collide after str() (e.g. {1: "a", "1": "b"}) previously lost values in the encoded mapping. Stateful __str__ implementations could also disagree across the reserved-key check, collision check, and final encode passes.

Fixes #8200 (checkpoint dict key collision after str).

Description & Review Guide

  • What are the major changes? Detect stringified-key collisions and fall back to pickle encoding; stringify each key once and reuse that string for reserved-key checks, collision detection, and the encoded mapping.
  • What is the impact of these changes? Prevents silent value loss for colliding keys and makes encode behavior consistent under stateful __str__.
  • What do you want reviewers to focus on? _encode dict path in _checkpoint_encoding.py and the regression tests in test_checkpoint_encode.py.

Related Issue

Fixes #8200

No other open PR targets this issue.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

@ktz03
fix(python): pickle checkpoint dicts with colliding str keys
3e719eb 
When dict keys collide after str() (e.g. 1 and "1"), JSON encoding silently overwrote values. Pickle those mappings instead (microsoft#8256).
Copilot AI balanced review requested due to automatic review settings September 10, 2026 22:58
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Sep 10, 2026
Copilot AI reviewed Sep 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused implementation resolves the reported data-loss case and includes appropriate regression coverage.

Pull request overview

Prevents checkpoint data loss when distinct dictionary keys stringify identically.

Changes:

  • Pickles mappings with stringified-key collisions.
  • Adds a restricted-decoding round-trip regression test.
File summaries
File Description
python/packages/core/agent_framework/_workflows/_checkpoint_encoding.py Detects collisions before dictionary encoding.
python/packages/core/tests/workflow/test_checkpoint_encode.py Verifies colliding keys retain both values.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@ktz03

LI (ktz03) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Fork CI for this PR may need a maintainer to Approve workflows before Python tests run (same as other community PRs). Happy to address review feedback once checks appear.

@FOWEPJF255

lsmlhi_25 (FOWEPJF255) commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Nice narrow fix for the str(k) key-collision case in checkpoint encoding 鈥?pickle fallback when len({str(k)}) != len(d) is easy to reason about and CI is green.

Should be ready for maintainer review on #8256.

@ktz03
fix(checkpoint): stringify dict keys once during encode
9dcccc3 
Reuse one str(key) for reserved-key checks, collision detection, and
the encoded mapping so stateful __str__ cannot disagree across passes.
@ktz03
fix(checkpoint): stringify dict keys once during encode
8ae90a0 
Reuse one str(key) for reserved-key checks, collision detection, and
the encoded mapping so stateful __str__ cannot disagree across passes.
@eavanvalkenburg

Eduard van Valkenburg (eavanvalkenburg) commented Sep 14, 2026

Copy link
Copy Markdown
Member

Thanks for the update. Before this is ready, could you please:

  • replace the PR body with the current repository template, retain its four required sections, provide substantive Motivation and Description answers, and complete the applicable checklist items

Once those are addressed, please re-request review. Thanks!

@ktz03

LI (ktz03) commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Eduard van Valkenburg (@eavanvalkenburg) Updated PR body to the repository template and addressed the latest review feedback on this head — please re-review when convenient.

@moonbox3
Evan Mattson (moonbox3) added this pull request to the merge queue Sep 15, 2026
Merged via the queue into microsoft:main with commit b45bcc3 Sep 15, 2026
40 checks passed
@baywet

Vincent Biret (baywet) commented Sep 15, 2026

Copy link
Copy Markdown
Member

Hi LI (@ktz03) ,

Is it possible the issue is in the description (8200) is not the correct one? Merging this PR ended up closing my PR (8200)

@ktz03

LI (ktz03) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for catching that — you're right.

The PR body incorrectly used Fixes #8200. The intended bug was #8256 (checkpoint dict key collision after str()), not your .NET configuration allow-list work on #8200.

From here, #8200 still shows as open on my side. If GitHub transiently marked it closed for you, please reopen / ignore the bogus cross-link — happy to help if anything is still stuck.

Sorry for the noise on your PR.

@ktz03 LI (ktz03) mentioned this pull request Sep 15, 2026
Closed

This branch was previously deployed

1 inactive deployment
github-app-auth — 546b9122 Deployed Sep 15, 2026 by moonbox3 via add_label #22850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@moonbox3 Evan Mattson (moonbox3) moonbox3 approved these changes
@chetantoshniwal chetantoshniwal Awaiting requested review from chetantoshniwal
@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) Awaiting requested review from eavanvalkenburg eavanvalkenburg is a code owner
@TaoChenOSU Tao Chen (TaoChenOSU) Awaiting requested review from TaoChenOSU
@jpalvarezl Jose Alvarez (jpalvarezl) Awaiting requested review from jpalvarezl jpalvarezl is a code owner
@giles17 Giles Odigwe (giles17) Awaiting requested review from giles17
@SergeyMenshykh SergeyMenshykh Awaiting requested review from SergeyMenshykh SergeyMenshykh is a code owner
@rogerbarreto Roger Barreto (rogerbarreto) Awaiting requested review from rogerbarreto rogerbarreto is a code owner
@westey-m westey (westey-m) Awaiting requested review from westey-m westey-m is a code owner

Assignees

No one assigned

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ktz03 @FOWEPJF255 @eavanvalkenburg @baywet @moonbox3