-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
Use client from LCP-SDK #5695
Use client from LCP-SDK #5695
Conversation
fa1b086
to
01bcb77
Compare
return f"{dataset_name}-{model_name}-{current_time}" | ||
|
||
|
||
async def arun_on_dataset( |
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.
Not a 1-way door, but current PR just exposes these as functions rather than extending the Client. Choice is whether the syntactic sugar of client.run_on_dataset()
instead of run_on_dataset(optional client)
is more useful than it is confusing (confusing here in that the client has diff methods depending on how you import)
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 either is fine tbh
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.
gotta love a good red diff
return f"{dataset_name}-{model_name}-{current_time}" | ||
|
||
|
||
async def arun_on_dataset( |
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 either is fine tbh
- Remove the client implementation (this breaks backwards compatibility for existing testers. I could keep the stub in that file if we want, but not many people are using it yet - Add SDK as dependency - Update the 'run_on_dataset' method to be a function that optionally accepts a client as an argument - Remove the langchain plus server implementation (you get it for free with the SDK now) We could make the SDK optional for now, but the plan is to use w/in the tracer so it would likely become a hard dependency at some point.
We could make the SDK optional for now, but the plan is to use w/in the tracer so it would likely become a hard dependency at some point.