chore(weave): basic sdk for human feedback spec types #2801
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=78b7a548e126099641750a088e3ad4a51065328c |
| from weave.trace_server.interface.base_object_classes import base_object_def | ||
|
|
||
|
|
||
| class HumanAnnotationColumn(base_object_def.BaseObject): |
There was a problem hiding this comment.
note: "HumanAnnotationColumn" is a permanent name. It is sort of like a table name, so we have to be sure we like it (: I think there is a subtle world where there are AnnotationColumns that are not filled out by Humans... which might mean we drop the human part?
There was a problem hiding this comment.
yep, that sounds right, do we then want a field within the class to denote that it came from a human? / is for a human
There was a problem hiding this comment.
There currently is creator and wb_user_id on the feedback table. I think using those are appropriate (which should be automatic right now)
|
|
||
| # If provided, this feedback type will only be shown | ||
| # when a call is generated from the given op | ||
| op_scope: Optional[list[str]] = None |
There was a problem hiding this comment.
is this expected to be a ref string or op name?
There was a problem hiding this comment.
I think we want ref strings? like the frontend format "weave:///griffin_wb/prod-evals-aug/op/Evaluation.summarize:*"
There was a problem hiding this comment.
i see in your tests that this is just the name itself. you might consider using a ref here. then if you wanted to match any version you can use weave:///e/p/name:* (just like we support with op_name in the call filter
There was a problem hiding this comment.
oh, you commented before i finished reviewing. Looks like we are on the same page
There was a problem hiding this comment.
haha yeah okay fixing
|
|
||
|
|
||
| class HumanAnnotationColumn(base_object_def.BaseObject): | ||
| json_schema: dict |
There was a problem hiding this comment.
this is probably fine for now. you could add a field validator that uses https://python-jsonschema.readthedocs.io/en/latest/validate/ to check for a schema error
| ) | ||
|
|
||
| @field_validator("json_schema") | ||
| def validate_json_schema(cls, v: dict) -> dict: |
There was a problem hiding this comment.
interesting. there are sort of two things here:
- if you want to explicitly type a subset of allowed json schemas (like you did here), just create a few BaseModels and make a union - no need for jsonschema.
- If you want to allow any json schema, do:
try:
jsonschema.validate(None,v)
except jsonschema.exceptions.SchemaError:
raise InvalidJson
except jsonschema.exceptions.ValidationError:
pass # we don't care that `None` does not conform
return v
| @@ -0,0 +1,58 @@ | |||
| from typing import Optional | |||
|
|
|||
| import jsonschema | |||
There was a problem hiding this comment.
did you need to add this package? I wonder if the trace server already has this transitively.
There was a problem hiding this comment.
I dont understand what this means... I see that the trace server has it in the requirements, how do I access it "transitively"?
There was a problem hiding this comment.
yeah, this is what I mean. in our pyproject.toml (inside core), there is no jsonschema. However, since litellm depends on jsonschema, you get it for free in trace server (when deployed).
There was a problem hiding this comment.
but for these CI tests to pass you need to add jsonschema to the dev reqs
There was a problem hiding this comment.
inside of
test = [
"nox",
"pytest>=8.2.0",
"pytest-asyncio>=0.23.6",
"pytest-cov>=5.0.0",
"pytest-xdist>=3.1.0",
"pytest-rerunfailures>=12.0",
"pytest-rerunfailures>=14.0",
"clickhouse_connect==0.7.0",
"fastapi>=0.110.0",
"sqlparse==0.5.0",
# Integration Tests
"pytest-recording>=0.13.2",
"vcrpy>=6.0.1",
# serving tests
"flask",
"uvicorn>=0.27.0",
"pillow",
"filelock",
"httpx",
]
| from weave.trace_server.interface.base_object_classes import base_object_def | ||
|
|
||
|
|
||
| class AnnotationColumn(base_object_def.BaseObject): |
There was a problem hiding this comment.
I keep going back and forth but I think this should actually be AnnotationSpec. Column doesn't really make that much sense.
Description
WB-21670
Basic SDK types for human feedback. Use the objects api to create a feedback spec, which can be used in the UI to generate human scorer columns
Testing
adds test. eventually we will want an integration test going from the type to loading the UI, but thats blocked on CI not sucking and the frontend landing