-
Notifications
You must be signed in to change notification settings - Fork 53
Python trace scoring candidate #1278
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
Make it easier to initialize an experiment's dataset reference without calling `initDataset`
ibolmo
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.
i think it's just otel that needs a follow-up, but check my other comments ...
| if expected is not None: | ||
| score = 1.0 if output == expected else 0.0 | ||
|
|
||
| if trace: |
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.
as a user why/when would trace ever be none?
| :return: A function that can be used as a task or scorer. | ||
| """ | ||
| # Disable span cache since remote function spans won't be in the local cache | ||
| _internal_get_global_state().span_cache.disable() |
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.
may be too indirect. if the remote function spans are not in the cache what's the big deal in checking the cache?
| assert state.span_cache.disabled is False | ||
|
|
||
| # Call init_function | ||
| f = init_function("test-project", "test-function") |
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.
it is a bit odd that loading a function will cause a cache to be disabled.
| root_span.log(output=output, metadata=metadata, tags=tags) | ||
|
|
||
| # Create trace object for scorers | ||
| from braintrust.trace import LocalTrace |
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 we have a circ. dep so i'd move this to the top of the file
| """ | ||
| self._otel_flush_callback = callback | ||
|
|
||
| async def flush_otel(self) -> 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.
where is this called? you probably need to find the right spot to call this and/or register_otel_flush
aside.. we solved this well with flush async. seems like we're in a similar pattern here.
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.
one option is to insert quickly in mermory and on need to flush you then block to flush.
| if not line: | ||
| continue | ||
| try: | ||
| record_dict = json.loads(line) |
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.
maybe consider some starts_with and maybe prepending the span_id/root_id... etc to avoid too many json.loads
| "root_span_id": self._root_span_id, | ||
| } | ||
|
|
||
| async def get_spans(self, span_type: Optional[list[str]] = None) -> list[SpanData]: |
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.
give the option to disable the cache
| span_parents=self.span_parents, | ||
| span_attributes=serializable_partial_record.get("span_attributes"), | ||
| ) | ||
| self.state.span_cache.queue_write(self.root_span_id, self.span_id, cached_span) |
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 could a performant problem if we write a lot to filesystem. would be nice to queue an amount and do bulk inserts to files to avoid read/write to filesystem
| SpanFetchFn = Callable[[Optional[list[str]]], Awaitable[list[SpanData]]] | ||
|
|
||
|
|
||
| class CachedSpanFetcher: |
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.
future should we consider fetchign and saving the data locally
No description provided.