Bedrock aws access key#849
Conversation
Bypassing mypy errors
leothomas
requested review from
a team,
jakelorocco and
nrfulton
as code owners
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
|
Link to issue: #564 |
…ea into bedrock_aws_access_key_id
| stream_logprobs = getattr(chunk.choices[0], "logprobs", None) | ||
| if stream_logprobs is not None: | ||
| if "logprobs" not in mot._meta: | ||
| mot._meta["logprobs"] = [] | ||
| mot._meta["logprobs"].append(stream_logprobs) |
There was a problem hiding this comment.
@ajbozarth can you chime in on whether this something we should add a mot field for instead of putting in _meta (cf GenerationMetadata)?
There was a problem hiding this comment.
It took some time to figure out exactly what I wanted to say, but I think I got Claude to summarize my thoughts correctly:
The pattern here would be mot.logprobs rather than adding it into GenerationMetadata
(which holds execution metadata: usage, model, provider, etc.). I'd lean toward
keeping it in _meta for now for two reasons:
- Type isn't settled. In streaming it accumulates as a list of per-chunk objects; in
non-streaming it's a single object. A public field needs a consistent, defined type. - Coverage is narrow. Logprobs are only returned when explicitly requested, and only
by some backends. The existing public fields onmotare things every backend always
populates.
If logprobs support expands to other backends, that would be the right time to define the
type and promote it to a real field — the same process the telemetry fields went through
before landing in GenerationMetadata.
|
@leothomas sorry we never got around to merging this in; I'll shepherd from here. Likely we can get it merged some time next week or so. |
|
No problem at all @nrfulton - thanks for following up! Let me know if there's anything I can assist with |
3 participants
Misc PR
Type of PR
Description
Testing