-
Notifications
You must be signed in to change notification settings - Fork 2
Flow complete fix #42
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
Conversation
fed5726 to
2240219
Compare
2240219 to
3a9e56d
Compare
| # THEN the span are returned bottom to top | ||
| assert read_from_opentelemetry_span(span=spans[1], key=HUMANLOOP_FILE_KEY)["tool"] | ||
| assert read_from_opentelemetry_span(span=spans[2], key=HUMANLOOP_FILE_KEY)["prompt"] | ||
| # assert read_from_opentelemetry_span(span=spans[3], key=HL_FILE_OT_KEY)["flow"] |
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.
No longer relying on TRACE_FLOW_CONTEXT here, using the ancestor to determine that logs are uploaded correctly.
|
|
||
| # Wait for the Prompt span to be exported; It was asynchronously waiting | ||
| # on the OpenAI call span to finish first | ||
| time.sleep(1) |
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.
Added some time.sleep in tests since the processor now waits async before sending the HL spans to processor
| assert flow_span_flow_metadata["is_flow_log"] | ||
|
|
||
| # THEN one of the flows is nested inside the other | ||
| spans: list[ReadableSpan] = [mock_export_method.call_args_list[i][0][0][0] for i in range(1, 5)] |
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.
Order of spans is no longer guaranteed due to async mechanism in processor, tests should no longer rely or test for order
c3cc96b to
11ff653
Compare
| self._upload_queue.task_done() | ||
|
|
||
| def _mark_span_completed(self, span_id: int) -> None: | ||
| for flow_log_span_id, flow_children_span_ids in self._flow_log_prerequisites.items(): |
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.
potentially dumb question: Right now we mark flow as complete only if self._flow_log_prerequisites.items() is not empty - is it always the case? Aka do we ever need to mark it as complete when self._flow_log_prerequisites.items() is empty
src/humanloop/otel/exporter.py
Outdated
| span.name, | ||
| parent_span_id, | ||
| ) | ||
| time.sleep(0.1) |
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.
discussed offline - if time.sleep potentailly increases latency - maybe remove it
| provider="openai", | ||
| max_tokens=-1, | ||
| temperature=0.7, | ||
| <<<<<<< HEAD |
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.
oups
| @@ -1,3 +1,6 @@ | |||
| [project] | |||
| name = "humanloop" | |||
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.
Is it necessary, aka why was it added now?
olehvell-h
left a comment
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.
make sense to me!
I haven't propertly understood all logic, so don't count to much on my review. At the same time I don't see a reason not to approve it. Maybe spend a bit of extra time on qaing just to make sure.
humanloop.promptorhumanloop.flow. This is also more dogmatic with how OT does thingsThings to look out for: I am introducing some async wait logic for the streaming case in the processor. I use a thread pool to not block the main thread. Please double-check if it makes sense and if there are no gotchas