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.

Sign up for GitHub

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

Jump to bottom

chore: add derived summary fields #2213

Merged
merged 32 commits into from
Sep 9, 2024
Merged

chore: add derived summary fields #2213

merged 32 commits into from
Sep 9, 2024

Conversation

gtarpenning
Copy link
Member

@gtarpenning gtarpenning commented Aug 26, 2024 •
edited
Loading

https://wandb.atlassian.net/browse/WB-20561

Add derived fields to the summary under the weave key, immediately after db read.

When a call is running, the derived fields will look like:

"summary": {
   "weave": {
      "status": "running",
      "trace_name": "name",
   }
}

When completed:

"summary": {
   "weave": {
      "status": "success",
      "trace_name": "name",
      "latency_ms": 1222,
      "usage": {....}
   }
}

@circle-job-mirror
Copy link

circle-job-mirror bot commented Aug 26, 2024 •
edited
Loading

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=3731fb7246cc78bcbd91deb3b3d6eeaba3fdbb21

@gtarpenning gtarpenning force-pushed the griffin/client-derived-fields-updated branch from 6cd0a28 to 8f29990 Compare August 27, 2024 23:04
@gtarpenning gtarpenning marked this pull request as ready for review August 29, 2024 17:43
@gtarpenning gtarpenning requested a review from a team as a code owner August 29, 2024 17:43
tssweeney
tssweeney reviewed Aug 29, 2024
View reviewed changes
weave/trace_server/trace_server_interface.py Outdated
@@ -45,7 +45,7 @@ class LLMCostSchema(LLMUsageSchema):

class WeaveSummarySchema(ExtraKeysTypedDict, total=False):
status: Optional[Literal["success", "error", "running"]]
nice_trace_name: Optional[str]
trace_name: Optional[str]
latency: Optional[int]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you comment what fidelity latency is? It is an int, so i am hoping it is at least ms

Copy link
Collaborator

Choose a reason for hiding this comment

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

in the past, i have favored using a suffix like latency_[UNIT] to make this very clear

tssweeney
tssweeney reviewed Aug 29, 2024
View reviewed changes
weave/tests/trace/test_client_trace.py Outdated
@@ -105,7 +116,7 @@ def test_trace_server_call_start_and_end(client):
start = tsi.StartedCallSchemaForInsert(
project_id=client._project_id(),
id=call_id,
op_name="test_name",
op_name="weave:///test_entity/test_project/op/test_name:test",
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: it should very much be allowed for op_name to NOT be a ref - does that still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, removing this change to make that explicit.

tssweeney
tssweeney reviewed Aug 29, 2024
View reviewed changes
weave/trace_server/trace_server_common.py Outdated
started_at_dt = _make_datetime_from_any(call_dict["started_at"])
ended_at_dt = _make_datetime_from_any(call_dict.get("ended_at"))

status = "success"
Copy link
Collaborator

Choose a reason for hiding this comment

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

recommend that you use constants over magic strings

tssweeney
tssweeney reviewed Aug 29, 2024
View reviewed changes
weave/trace_server/trace_server_common.py
weave_summary["latency"] = latency

summary["weave"] = weave_summary
return cast(tsi.SummaryMap, summary)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of casting, can you directly construct this?

Copy link
Member Author

@gtarpenning gtarpenning Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can't seem to construct this while keeping mypy happy, because while we control the weave key in the summary, there are also arbitrary user controlled keys. tsi.SummaryMap allows this (with some special pydantic foo) but mypy doesn't know that...

tssweeney
tssweeney reviewed Aug 29, 2024
View reviewed changes
weave/trace_server/trace_server_common.py Outdated
def _make_datetime_from_any(
dt: Optional[Union[str, datetime.datetime]],
) -> Optional[datetime.datetime]:
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we dealing with two different formats here? under what conditions are they different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ripped this out

@gtarpenning gtarpenning force-pushed the griffin/client-derived-fields-updated branch from 2fdd502 to 24a6dc3 Compare August 30, 2024 18:32
jwlee64
jwlee64 reviewed Sep 3, 2024
View reviewed changes
weave/trace_server/trace_server_common.py
FeedbackQueryRes,
Query,
)
from weave.trace_server import trace_server_interface as tsi
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe just add SummaryMap and TraceStatus

@gtarpenning gtarpenning merged commit 5a5f21d into master Sep 9, 2024
27 checks passed
@gtarpenning gtarpenning deleted the griffin/client-derived-fields-updated branch September 9, 2024 17:24
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwlee64 jwlee64 jwlee64 left review comments

@tssweeney tssweeney tssweeney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gtarpenning @tssweeney @jwlee64