feat: python API for feedback #1723
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=ea76eb517475f114846282b43b5b02c766bed606 |
jamie-rasmussen
force-pushed
the
jamie/feedback-python-api
branch
2 times, most recently
from
686acf6
to
2e75532
Compare
jamie-rasmussen
force-pushed
the
jamie/feedback-python-api
branch
2 times, most recently
from
edf2926
to
0792922
Compare
weave/weave_client.py
Outdated
| reaction: Optional[str] = None, | ||
| offset: int = 0, | ||
| limit: int = 100, | ||
| ) -> Feedbacks: |
There was a problem hiding this comment.
Can you please add documentation to this method? I see two things that might be confusing for me:
queryis implemented asquery_or_feedback_id. I think you made this decision in order to make the first positional argument dual purpose and therefore nicer syntax. Anyway, would be good to make that clearreaction-> is this supposed to be the literal emoji or shorthand? Both? Good to document
weave/feedback.py
Outdated
| if self.items: | ||
| self.items = [f for f in self.items if f.id != feedback_id] | ||
|
|
||
| def _repr_pretty_(self, p: Any, cycle: bool) -> None: |
There was a problem hiding this comment.
is this automatically called by ipython? maybe i am unfamiliar with is stub?
There was a problem hiding this comment.
Yes, this is called automatically by ipython. https://ipython.readthedocs.io/en/stable/config/integrating.html#rich-display
Shawn called out this pretty repr stuff as the bit he's most likely to want to change for consistency across APIs, but he was OK with it going in while he's trying out different possibilities.
weave/feedback.py
Outdated
| query=Query( | ||
| **{ | ||
| "$expr": { | ||
| "$eq": [ |
There was a problem hiding this comment.
might be nice if this also included the ref in the query expression. Else, you could inadvertently purge an id that does not belong to that ref
There was a problem hiding this comment.
Actually, I'm going to defer this - it's a good idea, but I'd have to first update the server side APIs, because also in the name of safety we put in restrictions on the purge endpoint that the purge query must be only a ref or list of refs.
weave/feedback.py
Outdated
| from weave.trace.refs import parse_uri | ||
|
|
||
|
|
||
| class Feedbacks: |
There was a problem hiding this comment.
I am seeing that we have 4 data structures:
tsi.Feedback-> The commonFeedbackdata pydantic schemaFeedbacks-> A container oftsi.Feedbacks- used primarily for display
- returned from
RefFeedback.query - Implements an iterator protocol over
tsi.Feedback
RefFeedback-> A "pointer" to feedback for a refable thing.- Implements an iterator protocol over
tsi.Feedback - Lazily fetches data
- Supports mutation operations (because it is relates to a specific "ref")
- Implements an iterator protocol over
- Implied:
Iterator[tsi.Feedback]
I'm not sure exactly why, but Feedbacks and RefFeedback seem to overlap in an important way.
Ok, i think either:
RefFeedbackwants to inherit fromFeedbacksRefFeedbackshould contain aFeedbacksinstead ofitems
For some reason i also feel like they are the same thing. RefFeedback and Feedbacks both can be thought of as FeedbackQuery. I think the only difference is that RefFeedback has the added property that you can add/purge. The other difference is that Feedbacks is pre-materialized, but i think you could also make Feedbacks lazy.
Ok, talking this out, consider changing to two classes: FeedbackQuery and RefFeedbackQuery(FeedbackQuery).
FeedbackQuery basically is merging of both RefFeedback and Feedbacks (except for add/purge). It is lazy, iterator, nicely prints, etc... It has an internal _query property holding the current query. It also has a query method that returns a new FeedbackQuery with the query append to the existing query.
Then, RefFeedbackQuery is a subclass with an additional weave_ref property. This is where add/purge goes.
The result of this is that the laziness/items cache/printing/iteration/query building/execution are all centralized and only the specific abilities of the RefFeedback are pulled out to the other class.
jamie-rasmussen
force-pushed
the
jamie/feedback-python-api
branch
from
0792922
to
3ff7ad6
Compare
jamie-rasmussen
force-pushed
the
jamie/feedback-python-api
branch
from
4e6f32a
to
33c6915
Compare
| return Refs(ref for ref in self.items if isinstance(parse_uri(ref), CallRef)) | ||
|
|
||
| # TODO: Perhaps there should be a Calls that extends AbstractRichContainer | ||
| def calls(self) -> list[TraceObject]: |
There was a problem hiding this comment.
2 notes:
- This could be optimized by grouping all the ids together then executing a single request instead of N network requests
- I feel like this wants to be on
FeedbackQueryitself, so that my code is:
client.feedback(...).calls()
There was a problem hiding this comment.
Thanks, can definitely address these in a followup.
Adds feedback APIs to the Python SDK.
Note: So far I have only added the feedback object to Calls and added querying for feedback at the project level. We would want to add feedback to objects like Datasets in a subsequent PR.
Some examples of use: