-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add local timestamps to request and response models - include provider timestamp in provider_details
#3598
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
base: main
Are you sure you want to change the base?
Conversation
Clarify that agents can be produced by a factory function if preferred.
The ModelRequest snapshots for Google/Vertex URL input tests were missing the timestamp field that was added in the parent commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
|
||
| _: KW_ONLY | ||
|
|
||
| timestamp: datetime = field(default_factory=_now_utc) |
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 don't think this should have a default, as it would be filled in for old requests without this field when they are deserialized and give the false impression that this date is accurate.
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.
that makes sense
| raw_finish_reason = candidate.finish_reason | ||
| if raw_finish_reason: # pragma: no branch | ||
| vendor_details = {'finish_reason': raw_finish_reason.value} | ||
| if response.create_time is not None: |
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.
This should be done separately from whether this a finish reason
| _model_name=first_chunk.model_version or self._model_name, | ||
| _response=peekable_response, | ||
| _timestamp=first_chunk.create_time or _utils.now_utc(), | ||
| _timestamp=_utils.now_utc(), |
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.
Could this be a default or something, so we don't need to repeat this?
| def _process_response(self, response: chat.ChatCompletion) -> ModelResponse: | ||
| """Process a non-streamed response, and prepare a message to return.""" | ||
| timestamp = number_to_datetime(response.created) | ||
| timestamp = _utils.now_utc() |
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.
We can drop this right, because the field already has a default?
| _provider_name=self._provider.name, | ||
| _provider_url=self._provider.base_url, | ||
| # type of created_at is float but it's actually a Unix timestamp in seconds | ||
| _provider_timestamp=int(first_chunk.response.created_at), |
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.
Why aren't we using number_to_datetime anymore?
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.
we set _provider_timestamp which is an int, when we pass that into the provider_details we do the conversion to datetime
this cast makes it seem like we're losing precision here but in reality those are seconds, don't know why the streaming model has it as a float
will double-check in case I oversaw something but this is intentional
| # After the chunk with native_finish_reason 'completed', OpenRouter sends one more | ||
| # chunk with usage data (see cassette test_openrouter_stream_with_native_options.yaml) | ||
| # which has native_finish_reason: null. Since provider_details is replaced on each | ||
| # chunk, we need to carry forward the finish_reason from the previous chunk. |
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.
Why was this not necessary previously?
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.
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.
just from following the logic it seems that: on the last chunk _map_provider_details was returning an empty dict, so it wasn't overriding the provider_details. Now the dict is by default not empty (because of the timestamp) so it overrides the finish_reason: 'completed' with None.
Makes me wonder now if the we should have separate provider_details['timestamp'] for the first and last chunks?
| if isinstance(part, TextContent): | ||
| return part.text | ||
| elif isinstance(part, ImageContent | AudioContent): | ||
| elif isinstance(part, (ImageContent, AudioContent)): |
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.
Please restore this, unions ftw
| assert stream.provider_details == snapshot( | ||
| { | ||
| 'timestamp': datetime.datetime(2025, 11, 2, 6, 14, 57, tzinfo=datetime.timezone.utc), | ||
| 'finish_reason': 'completed', | ||
| 'cost': 0.00333825, | ||
| 'upstream_inference_cost': None, | ||
| 'is_byok': False, | ||
| 'downstream_provider': 'xAI', | ||
| } | ||
| ) |
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.
idk why this wasn't a problem before but from the snapshot alone it seems like this test hasn't been reran in a while, right?
- remove default timestamp on ModelResponse and set it manually - adjust tests
| timestamp: datetime | None = None | ||
| """The timestamp when the request was sent to the model.""" | ||
|
|
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.
ModelRequest.timestamp needs to be None by default for backwards compat
docs/agents.md
Outdated
|
|
||
| !!! tip "Agents are designed for reuse, like FastAPI Apps" | ||
| Agents are intended to be instantiated once (frequently as module globals) and reused throughout your application, similar to a small [FastAPI][fastapi.FastAPI] app or an [APIRouter][fastapi.APIRouter]. | ||
| Agents can be instantiated once as a module global and reused throughout your application, similar to a small [FastAPI][fastapi.FastAPI] app or an [APIRouter][fastapi.APIRouter], or be created dynamically by a factory function like `get_agent('agent-type')`, whichever you prefer. |
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.
Remove this from this PR please ;)
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.
this will go away when I update the branch
| parts.append(_messages.UserPromptPart(self.user_prompt)) | ||
|
|
||
| next_message = _messages.ModelRequest(parts=parts) | ||
| next_message = _messages.ModelRequest(parts=parts, timestamp=now_utc()) |
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.
Let's se it just once on next_message below like we do with instructions, instead of adding it to 2 constructors
| assert not self._did_stream, 'stream() should only be called once per node' | ||
|
|
||
| model_settings, model_request_parameters, message_history, run_context = await self._prepare_request(ctx) | ||
| self.request.timestamp = now_utc() |
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.
Actually if we have it here we don't need to set it above right?
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.
yeah that makes sense, I wasn't totally sure about this line bc I ran into an issue with the temporal tests, but that issue was unrelated so this can stay. I'll remove the unnecessary assignments
| instructions = await ctx.deps.get_instructions(run_context) | ||
| self._next_node = ModelRequestNode[DepsT, NodeRunEndT]( | ||
| _messages.ModelRequest(parts=[], instructions=instructions) | ||
| _messages.ModelRequest(parts=[], instructions=instructions, timestamp=now_utc()) |
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.
Same as above; if we set the timestamp already before sending model.request, we don't need it here right?
| merged_message = _messages.ModelRequest( | ||
| parts=parts, | ||
| instructions=last_message.instructions or message.instructions, | ||
| parts=parts, instructions=last_message.instructions or message.instructions, timestamp=now_utc() |
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.
We should definitely not create a new timestamp here, but use one of the existing ones
Closes #1996
This is an attempt at adding a timestamp field directly on
ModelRequest. This PR also overrides thetimestampinModelResponsethat is set to the value returned by the provider (createdorcreated_atin most cases).Previously, only the response got a timestamp. The request's timestamp was set by the first part. The problem main with this approach is that some providers, like openai, return unix time (seconds passed since unix epoch), which for very fast requests can mean (as far as the user sees), that the response's timestamp is earlier that the request's.
Thus for absolute certainty that this is not the case, this PR sets both timestamps locally. The provider's timestamp is still stored in the models, in the response's
provider_details['timestamp'].Many tests needed adjusting because the timestamp field now becomes part of every snapshot. If this is undesired we could set the timestamp to be a private field in the model? I don't know exactly how that would go but there must be a way :).
Note: I see one commit from #3592 snuck into here, will be irrelevant when that merges.