Skip to content

Fix docker issues, TTL cleanup #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 12, 2025
Merged

Conversation

abrookins
Copy link
Contributor

This PR fixes an issue with local testing and Docker where redis containers would fail to come up. Previously, each pytest xdist worker would attempt to connect to its own redis instance. If the instance failed to come up in time, the configuration defaulted to localhost: 6379. As long as a redis instance is running locally, the problem would have been masked, but this causes the tests to fail when a local redis instance is unavailable.

We attempt to resolve the issue by waiting longer for individual redis containers to become available. In the previous configuration, it was also possible for multiple xdist workers to start redis instances mapped to the same port (causing conflicts). We resolve this by giving each xdist worker a random, non-conflicting host port for their redis instance.

@abrookins abrookins requested review from Copilot and bsbodden June 12, 2025 20:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses Docker issues encountered during local testing by updating Redis container port mapping and improves TTL handling in Redis-based checkpoints. Key changes include:

  • Updating the Docker Compose Redis service configuration to use an ephemeral host port.
  • Refactoring TTL logic in checkpoint functions to delegate TTL application to a helper function.
  • Reordering and consolidating import statements in tests and checkpoint modules.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_cluster_mode.py Reorganized import order for clarity.
tests/test_async_cluster_mode.py Adjusted import order to correctly locate AsyncRedisStore.
tests/docker-compose.yml Updated Redis service port mapping to use a published ephemeral port.
tests/conftest.py Added connection wait logic to ensure the Docker container becomes ready.
langgraph/checkpoint/redis/types.py Updated import order for RedisCluster to match expected usage.
langgraph/checkpoint/redis/aio.py Refactored TTL handling in async functions to use a helper function.
langgraph/checkpoint/redis/init.py Minor import reordering for consistency.
Comments suppressed due to low confidence (3)

langgraph/checkpoint/redis/aio.py:598

  • Ensure that _apply_ttl_to_keys is implemented to properly handle a null second parameter and correctly apply TTL when no blob keys are provided.
await self._apply_ttl_to_keys(checkpoint_key, [key for key, _ in blobs] if blobs else None,)

langgraph/checkpoint/redis/aio.py:768

  • Verify that _apply_ttl_to_keys handles cases when only a single key is present to avoid inadvertently skipping TTL assignment for created keys.
await self._apply_ttl_to_keys(created_keys[0], created_keys[1:] if len(created_keys) > 1 else None,)

tests/docker-compose.yml:7

  • Include a comment explaining that 'published: 0' instructs Docker to dynamically assign an ephemeral port, which supports non-conflicting port allocation during parallel testing.
published: 0

@abrookins abrookins merged commit a56a6b7 into main Jun 12, 2025
20 checks passed
@abrookins abrookins deleted the fix/fix-more-async-cluster-stuff branch June 12, 2025 20:48
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.

2 participants