Skip to content
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

add metadata event to /stream for collecting run_id of individual runs #180

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

jakerachleff
Copy link
Contributor

@jakerachleff jakerachleff commented Nov 7, 2023

Adds a new event with type metadata that can return metadata about a run. This can be used for getting a run_id from the /stream API

Testing

  • Unit Tests: added in test_server_client
  • Manual Tests: I ran examples/llm/server.py locally and ensured the returned run_id from the metadata event matched the one logged to langsmith

@cla-bot cla-bot bot added the cla-signed label Nov 7, 2023
@jakerachleff jakerachleff changed the title add metadata event to streaming API add metadata event to /stream for collecting run_id of individual runs Nov 7, 2023
@@ -681,10 +681,23 @@ async def _stream() -> AsyncIterator[dict]:
) from validation_exception

try:
config_w_callbacks = config.copy()
event_aggregator = AsyncEventAggregatorCallback()
config_w_callbacks["callbacks"] = [event_aggregator]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooc: should we be merging rather than overwriting these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @eyurtsev since I'm copying his pattern here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically was fine to overwrite. With addition of per request modifier, we might want to merge if a user is adding custom callbacks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say keep as is for now and we can follow up with a separate PR to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me, will merge

if not has_sent_metadata and event_aggregator.callback_events:
yield {
"event": "metadata",
"data": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need to serialize the data into json? I think it doesn't do this by default for streaming

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that's a good callout - that makes sense why the output had single quotes instead of double quotes! I'll make this change before pushing

Copy link
Contributor Author

@jakerachleff jakerachleff Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before:
Screenshot 2023-11-06 at 9 52 08 PM

After:
Screenshot 2023-11-06 at 9 51 49 PM

@jakerachleff jakerachleff merged commit cf6935d into main Nov 7, 2023
7 checks passed
@jakerachleff jakerachleff deleted the 2023-11-06-stream-runid branch November 7, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants