-
Notifications
You must be signed in to change notification settings - Fork 24
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! |
tests/Helper/UuidAssertionTrait.php
Outdated
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 😄 |
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
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
MessageInterface
now requires thegetId(): Uuid
method to be implemented by all message classes.Implementation Details
symfony/uid
package dependency (^6.4 || ^7.1)public readonly Uuid $id
property 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