-
Notifications
You must be signed in to change notification settings - Fork 23
feat!: add UUID to all messages #364
Conversation
7f5b2f8 to
54c82db
Compare
b8c85bd to
5191313
Compare
|
@chr-hertel I have a question regarding the normalized message format: Currently, the normalizers include the UUID as an Should we:
I'm leaning towards option 2 to keep the library focused, but wanted to get your thoughts on this, knowing that I need the timestamp in my project 😄 Example of what option 1 would look like: public function normalize(mixed $data, ?string $format = null, array $context = []): array
{
return [
'role' => $data->getRole()->value,
'content' => $data->content,
'id' => $data->getId()->toRfc4122(),
'timestamp' => $data->getId()->getDateTime()->format('c'), // ISO 8601 format
];
}What do you think? |
5b00f02 to
dea59a2
Compare
|
I would say go with option 2 to keep the library as minimal as needed. Having the UUIDv7 is already allowing people to extract the timestamp when overriding the normalizer - as shown in the example. It is sortable by this data and also extractable on demand afterwards when someone needs it and do not want to have an overwritten normalizer. So flexibility is there. As a compromize, when moving on with option one, it would be nice to have the format configurable with the normalizer context because the ISO 8601 format is not the only one utilized and so it would be more flexible. |
|
Great discussion! I think Dennis's suggestion for configurable timestamps is excellent and would provide the flexibility needed. However, I believe we should proceed step by step. This PR already implements the core UUID functionality and ID serialization, which is the foundation. Let's get this merged first, then we can tackle the configurable timestamp feature in a follow-up PR. The current implementation gives us:
Once this is merged, we can discuss and implement the configurable timestamp context approach in a separate, focused PR. What do you think? |
|
Sounds pretty good to me. Options would then only have to be transported to the normalizers context. But it could be a good next step, as you mentioned. I already saw the code you had here as an example before editing your message and it looked also good. The only thing that i would then think about is the options transport. Currently it is this flexible array construction but maybe the options also need a bit more brain to have it maybe more structured into different features with a context class, or something in this direction but this is also a topic on it's own. Generally, even i was against having the |
|
You mean this code, right ❓ For reference, here's how we could implement the configurable timestamp feature in a follow-up PR: // Example implementation for SystemMessageNormalizer
public function normalize(mixed $data, ?string $format = null, array $context = []): array
{
$array = [
'role' => $data->getRole()->value,
'content' => $data->content,
'id' => $data->getId()->toRfc4122(),
];
// Add timestamp if requested in context
if ($context['include_timestamp'] ?? false) {
$timestampFormat = $context['timestamp_format'] ?? 'c'; // ISO 8601 as default
$array['timestamp'] = $data->getId()->getDateTime()->format($timestampFormat);
}
return $array;
}Usage examples: // Default behavior (no timestamp)
$normalizer->normalize($message);
// Include ISO 8601 timestamp
$normalizer->normalize($message, null, ['include_timestamp' => true]);
// Include Unix timestamp
$normalizer->normalize($message, null, [
'include_timestamp' => true,
'timestamp_format' => 'U'
]);
// Include custom format
$normalizer->normalize($message, null, [
'include_timestamp' => true,
'timestamp_format' => 'Y-m-d H:i:s'
]);This approach would need to be applied to all message normalizers (User, System, Assistant, ToolCall) for consistency. |
|
Yep! That is it what i saw with the last message - without the examples. Thanks for re-referencing it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this more, than having the preg match written over and over again
56c7c92 to
e3e1439
Compare
90f7995 to
14ac0ba
Compare
|
CI 💚 |
14ac0ba to
07b8111
Compare
|
So I could get an approval from you @DZunke ? 😄 |
|
@OskarStark sure, i scrolled through the code a second time and yeah, it looks fine from my side. Thanks again! |
|
So, @OskarStark, how much of code and comments were hand crafted and what AI? 😆 |
|
Everything is AI, except 3 of my comments 😄 |
chr-hertel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normalizers are intended for the contract of the API payload - so we cannot just add the ID there. OpenAI might tolerate that, but for example Mistral and Anthropic fail with invalid request errors.
The UID is intended for user land code anyways, and i guess after a revert in normalizers this is good to be merged :)
499c48e to
a6d73c1
Compare
All message types now include a UUID v7 identifier that provides: - Unique identification for each message - Embedded timestamp information - Sortable message ordering The UUID is available on message objects for userland code but is not serialized in API payloads to ensure compatibility with all LLM providers (Mistral and Anthropic fail with invalid request errors when extra fields are present). BREAKING CHANGE: Message constructors now generate a UUID automatically 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
a6d73c1 to
60763c7
Compare
chr-hertel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice one - finally settled :D thanks @OskarStark
|
Can we please get a release? 😄 So @llupa can proceed? Thanks |
This PR was merged into the main branch. Discussion ---------- feat!: add UUID to all messages | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | yes | Issues | | License | MIT Cherry picking php-llm/llm-chain#364 Commits ------- 4fa1279 feat!: add UUID to all messages (#364)
This PR was merged into the main branch. Discussion ---------- feat!: add UUID to all messages | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | yes | Issues | | License | MIT Cherry picking php-llm/llm-chain#364 Commits ------- 4fa1279 feat!: add UUID to all messages (#364)
Summary
Breaking Change 🚨
This is a breaking change as
MessageInterfacenow requires thegetId(): Uuidmethod to be implemented by all message classes.Implementation Details
symfony/uidpackage dependency (^6.4 || ^7.1)public readonly Uuid $idproperty to all message classesUuid::v7()getId()method to all message implementationsWhy UUID v7?
UUID v7 offers significant advantages over other UUID versions:
Practical Example
Test Coverage
Added tests for each message type to ensure:
Closes #77
Closes #344
🤖 Generated with Claude Code