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

feat: phoenix client get_spans_dataframe() and query_spans() #2151

Merged
merged 22 commits into from
Feb 5, 2024

Conversation

RogerHYang
Copy link
Contributor

@RogerHYang RogerHYang commented Jan 31, 2024

resolves #2091
resolves #2136
resolves #2137

Screenshot shows the client in a console returning dataframes from Phoenix running in the background.

Screenshot 2024-01-30 at 4 18 49 PM

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jan 31, 2024
@RogerHYang RogerHYang changed the title feat: phoenix client feat: phoenix client get_spans_dataframe() and query_spans() Jan 31, 2024
@@ -0,0 +1,127 @@
import asyncio
Copy link
Contributor

Choose a reason for hiding this comment

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

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 see. Thanks for the tip.

)


class QuerySpansHandler(HTTPEndpoint):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you consolidate these two? It seems like they could be just /v1/spans?format=xyz

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 give it some thought. But probably will do this in a follow-up PR for refactoring

traces: Traces
evals: Optional[Evals] = None

async def post(self, request: Request) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking but POST is not the most standard for REST. Getting something is usually "GET" - I know we use graphql which breaks that convention but it might make sense to stick with conventions in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. In the interest of time, i'll get this deployed first. Then I'll enumerate all the refactoring tasks in a follow-up PR, because I need time to understand them better myself, but those changes don't impact the user.

logger = logging.getLogger(__name__)


class Client:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need a shared protocol with Session so this can be say this complies with all the session.evaluation helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 1, 2024
@RogerHYang RogerHYang merged commit e44b948 into main Feb 5, 2024
9 checks passed
@RogerHYang RogerHYang deleted the phoenix-client branch February 5, 2024 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
2 participants