Improve trajectory storage and fix schema handling - #30
Conversation
📝 WalkthroughWalkthroughThe PR enables non-string entity content by serializing/deserializable payloads in Milvus and filesystem search, expands schema types to allow str|list|dict, consolidates per-message Phoenix sync into a single trajectory entity, and adds an Architecture section to the README. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Phoenix as PhoenixSync
participant Serializer as Serializer
participant Milvus as MilvusBackend
participant DB as SQLite/Storage
Client->>Phoenix: send messages / trajectory
Phoenix->>Serializer: prepare trajectory entity (messages list, metadata)
Serializer->>Milvus: serialize content -> compute embeddings -> upsert
Milvus->>DB: persist metadata/index (storage layer)
Client->>Milvus: search query
Milvus->>Serializer: serialize query / match against serialized content
Milvus->>DB: retrieve matching entity ids/records
DB->>Serializer: return stored serialized content
Serializer->>Client: deserialize and return original content shapes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kaizen/backend/milvus.py`:
- Around line 130-137: The int(update.id) cast in the milvus.upsert call can
raise ValueError for non-numeric IDs; wrap the conversion in defensive
validation inside the function that calls milvus.upsert (the code block invoking
self.milvus.upsert) by attempting to convert update.id with a try/except (or
using str.isdigit/explicit validation) and on failure log the invalid ID and
skip or map it to a safe fallback id rather than letting the exception
propagate; ensure the logged message includes update.id, namespace_id, and the
event type so you can trace which record was rejected and keep the upsert
payload shape (keys like 'id', 'type', 'content', 'embedding', 'metadata')
unchanged when a valid id is present.
🧹 Nitpick comments (2)
kaizen/backend/milvus.py (2)
113-114: Verify semantic search quality with serialized structured content.When
entity.contentis a dict/list, the serialized JSON string (e.g.,'{"key": "value"}') is used for embedding-based search. This may reduce search quality since JSON syntax tokens don't carry semantic meaning.Consider whether a dedicated text representation (e.g., extracting key text fields) would improve search relevance for structured content.
198-198: Schema change requires migration for existing collections.Increasing
max_lengthfrom 512 to 65535 only affects newly created collections. Existing Milvus collections will retain the old schema and may reject content exceeding 512 characters.Consider adding a migration strategy or documenting the upgrade path for existing deployments.
| self.milvus.upsert(collection_name=namespace_id, data={ | ||
| 'type': entity_type, | ||
| 'id': update.id, | ||
| 'content': update.content, | ||
| 'id': int(update.id), | ||
| 'content': content_str, | ||
| 'created_at': int(now.timestamp()), | ||
| 'embedding': self.embedding_model.encode(update.content), | ||
| 'embedding': self.embedding_model.encode(content_str), | ||
| 'metadata': update.metadata | ||
| }, partial_update=True) |
There was a problem hiding this comment.
Potential ValueError if update.id is non-numeric.
The int(update.id) conversion on line 132 will raise ValueError if the ID is not a valid integer string. While the conflict resolution logic should only return UPDATE events for existing entities with numeric IDs, consider adding defensive error handling.
Suggested defensive handling
case 'UPDATE':
+ try:
+ entity_id = int(update.id)
+ except ValueError:
+ logger.error(f"Invalid entity ID for UPDATE: {update.id}")
+ continue
self.milvus.upsert(collection_name=namespace_id, data={
'type': entity_type,
- 'id': int(update.id),
+ 'id': entity_id,
'content': content_str,🤖 Prompt for AI Agents
In `@kaizen/backend/milvus.py` around lines 130 - 137, The int(update.id) cast in
the milvus.upsert call can raise ValueError for non-numeric IDs; wrap the
conversion in defensive validation inside the function that calls milvus.upsert
(the code block invoking self.milvus.upsert) by attempting to convert update.id
with a try/except (or using str.isdigit/explicit validation) and on failure log
the invalid ID and skip or map it to a safe fallback id rather than letting the
exception propagate; ensure the logged message includes update.id, namespace_id,
and the event type so you can trace which record was rejected and keep the
upsert payload shape (keys like 'id', 'type', 'content', 'embedding',
'metadata') unchanged when a valid id is present.
- Add architecture diagram to README - Store trajectory as single entity per span/trace ID - Support list and dict content types (not just strings) - Fix Milvus backend to handle non-string content - Fix SQLite schema for new content format
illeatmyhat
left a comment
illeatmyhat
left a comment
There was a problem hiding this comment.
seems ok, as long as we can handle updating incomplete trajectories
i wonder if it would be practical to let people define a callback for the embedding string
as it is, making an embedding of an entire trajectory is probably meaningless for search purposes.
we would likely prefer it to be the user’s first utterance or some summary of the whole conversation, and need some mechanism to allow people to do this.
|
@illeatmyhat I agree we should index by the utterance or sub-task. Will work on it next. |
2 participants
For #28
Summary by CodeRabbit
Documentation
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.