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

chore(weave): human annotation labeling review comments #3020

Merged
merged 12 commits into from
Nov 19, 2024

Conversation

gtarpenning
Copy link
Member

@gtarpenning gtarpenning commented Nov 19, 2024

Description

Crush a bunch of human feedback rough edges.

This pr:

  • Sidebar:

    • Store boolean types as boolean, not string
    • add better loading state, don't show CTA when still loading
    • add analytics event
    • increase "save" button padding
  • Trace table:

    • fix paging to keep feedback expanded when changing calls
    • swap icon position (feedback/trace expand)
    • hide/show feedback sidebar -> hide/show feedback
    • correctly display bool types in trace table
  • Create form:

    • Add integer type (disallows float)
    • "enum" -> "options"
    • fix annotation form default state
    • description optional
    • zod number min/max -> minimum/maximum
    • after scorer creation, set form data to null

@circle-job-mirror
Copy link

circle-job-mirror bot commented Nov 19, 2024

@gtarpenning gtarpenning marked this pull request as ready for review November 19, 2024 21:10
@gtarpenning gtarpenning requested review from a team as code owners November 19, 2024 21:10
case 'integer':
return (
<NumericalFeedbackColumn
min={jsonSchema.minimum}
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure your are respecting https://json-schema.org/understanding-json-schema/reference/numeric#range correctly (inclusive vs non inclusive)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it should be inclusive by default, which is what the component expects. Rejecting iff: (min != null && parsedVal < min) || (max != null && parsedVal > max) so accepting when val == min

@gtarpenning gtarpenning merged commit 3fa8ebf into master Nov 19, 2024
116 of 118 checks passed
@gtarpenning gtarpenning deleted the griffin/human-feedback-feedback branch November 19, 2024 22:53
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants