fix: graph cache serialization of runtime components#13232
Conversation
WalkthroughVertex.getstate now clears the ChangesGraph vertex serialization cleanup
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lfx/tests/unit/graph/graph/test_base.py (1)
110-130: ⚡ Quick winConsider adding round-trip serialization test for cache path validation.
The test successfully verifies that pickling no longer raises when a vertex contains an unserializable component. However, since this fix targets the Redis/Celery cache path (serialize → cache → retrieve → deserialize), consider also testing the full round-trip:
deserialized_payload = pickle.loads(serialized_payload) deserialized_graph = deserialized_payload["result"] assert deserialized_graph.vertices[0].custom_component is None # Optionally: verify the graph remains functional after deserializationThis would confirm that:
- Unpickling succeeds and produces a valid graph
- The
custom_componentfield is correctly restored to None- Cached graphs can be used after retrieval
The current test correctly validates the serialization behavior, but extending it to cover deserialization would provide stronger confidence that the cache round-trip works end-to-end.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lfx/tests/unit/graph/graph/test_base.py` around lines 110 - 130, Update the test_graph_cache_serialization_omits_runtime_component_state to perform a full serialize→deserialize round trip: use pickle.loads on serialized_payload to produce deserialized_payload, extract deserialized_graph = deserialized_payload["result"], and assert deserialized_graph.vertices[0].custom_component is None (and optionally exercise a simple graph operation to ensure deserialized_graph remains functional). This adds cache-path validation by confirming pickle.loads succeeds and the runtime component was cleared on restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lfx/tests/unit/graph/graph/test_base.py`:
- Around line 110-130: Update the
test_graph_cache_serialization_omits_runtime_component_state to perform a full
serialize→deserialize round trip: use pickle.loads on serialized_payload to
produce deserialized_payload, extract deserialized_graph =
deserialized_payload["result"], and assert
deserialized_graph.vertices[0].custom_component is None (and optionally exercise
a simple graph operation to ensure deserialized_graph remains functional). This
adds cache-path validation by confirming pickle.loads succeeds and the runtime
component was cleared on restore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb9914a4-f704-43a2-b754-98ccf2e31476
📒 Files selected for processing (2)
src/lfx/src/lfx/graph/vertex/base.pysrc/lfx/tests/unit/graph/graph/test_base.py
0ba124b to
b03765b
Compare
🧩 Analysis chain🏁 Script executed: Length of output: 256 🏁 Script executed: Length of output: 161 🏁 Script executed: Length of output: 3909
|
|
CI follow-up: the required
I tried to rerun the failed CI jobs, but GitHub requires repository admin rights for that rerun. |
Fixes #8476
Summary
This avoids storing live runtime component instances in cached graph state.
When the Redis/Celery path caches a
Graph, eachVertexis serialized as part of that graph. Some runtime component instances can hold non-serializable state such as thread-local console objects, which can make cache serialization fail even for simple built-in flows.The patch omits
custom_componentfromVertex.__getstate__. The component instance is runtime state and can be re-instantiated from vertex data during build.Scope
src/lfx/src/lfx/graph/vertex/base.pyandsrc/lfx/tests/unit/graph/graph/test_base.py.Regression Coverage
The test injects an unserializable runtime component into a graph vertex, serializes a payload containing the graph, deserializes it, and asserts that
custom_componentis omitted from both the serialized state and the restored graph vertex.Validation
uv run ruff check src/lfx/graph/vertex/base.py tests/unit/graph/graph/test_base.pyuv run pytest tests/unit/graph/graph/test_base.py -q(16 passed, 1 skipped)