Skip to content

fix(checkpoint): apply list(before=...) to LangGraph's UUIDv6 checkpoint ids - #206

Open
ericdelorefice wants to merge 1 commit into
redis-developer:mainfrom
ericdelorefice:before-uuid6
Open

ericdelorefice wants to merge 1 commit into
redis-developer:mainfrom
ericdelorefice:before-uuid6

Conversation

@ericdelorefice

Copy link
Copy Markdown

Problem

list(before=...) builds its filter by parsing the before checkpoint id as a ULID, and when the parse fails it drops the filter:

except Exception:
    # If not a valid ULID, ignore the before filter
    pass

LangGraph creates checkpoint ids with uuid6(), so the parse fails for every checkpoint a graph writes, and get_state_history(config, before=...) returns the whole history. All four savers do this. test_list_before_with_uuid6_no_warning notes it: "The current implementation silently ignores non-ULID 'before' IDs". #136 removed the warning, but the filter is still skipped.

Reproduction

A one-node graph on RedisSaver against Redis 8, three turns, then get_state_history(config, before=<the middle checkpoint>), on main:

history: 9 checkpoints; before the middle one: 9 returned, 5 of them at or after it

Change

A new helper, checkpoint_id_timestamp() in util.py, returns the time a checkpoint id encodes:

  • a ULID returns ULID.timestamp, the value the savers already store for one
  • a UUIDv6 returns its 60-bit timestamp in milliseconds since the epoch, the unit already stored for non-ULID ids
  • anything else, such as a uuid4, returns None

All four savers now use it in two places. put/aput use it to stamp checkpoint_ts, falling back to the current time (full savers) or checkpoint["ts"] (shallow savers) as before. list/alist use it to build the before filter. Stamping and filtering come from the same id, so a checkpoint saved in the background after the next one was created is still placed correctly. A before id that encodes no time is still not filtered, as today.

Two things I noticed and left alone:

  • ULID.timestamp is in seconds, while the comments said milliseconds and the fallback stores milliseconds. I kept ULIDs exactly as they are so existing ULID data still compares correctly.
  • Checkpoints written before this change with UUIDv6 ids were stamped with the write time rather than the id time. Those values are within milliseconds of each other, so before on old data is approximate at the boundary and exact for new data.

#186 also touches before for non-ULID ids, as part of a larger change that has been open since April and now conflicts with main. It post-filters in Python over the first 10,000 documents. This PR only fixes before and keeps the filter in the index. If you would rather take #186, I am happy to close this.

Verification

New tests/test_list_before_uuid6.py: the reproduction above for RedisSaver and AsyncRedisSaver, asserting before returns exactly the checkpoints whose id sorts before the middle one, and a unit test of the helper. With the source reverted to main and the tests kept, both end-to-end tests fail. The helper test fails there too, but only because the helper is new.

Full suite with the repo's docker-compose Redis, tests/integration and tests/sentinel excluded:

  • with this change: 717 passed, 13 skipped
  • main: 714 passed, 13 skipped (the same run minus the 3 new tests)

poetry run format, sort-imports and check-mypy are clean.

…int ids

The before filter parsed the checkpoint id as a ULID and dropped the
filter when that failed. LangGraph creates ids with uuid6(), so
get_state_history(before=...) returned the whole history. A shared
helper now reads the timestamp out of a ULID or a UUIDv6 id, and the
savers use it both to stamp checkpoint_ts and to build the before
filter, so the two agree.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant