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

Link labels and evals with a new column #243

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Nov 25, 2024

  • New column on evaluation results to link with the label it originates from
  • Rename label to label_class_name (username) instead of label_class_name_labeling_queue_name
  • Handle label additions and deletions so that we delete evaluation results when the corresponding label is deleted
  • Order label values by their numeric score value when displaying
  • minor refactor in the backend

Important

This PR adds a label_id column to link evaluation results with labels, updates label handling and format, and includes a minor backend refactor.

  • Database Changes:
    • Add label_id column to evaluation_scores table.
    • Update workspace_usage table to add primary key and unique constraint on workspace_id.
  • Label Handling:
    • Rename label format to label_class_name (username).
    • Handle label additions and deletions to update evaluation results accordingly.
  • Frontend Changes:
    • Order label values by numeric score in labels.tsx and add-label-popover.tsx.
    • Update label-related components to reflect new label format and operations.
  • Backend Refactor:
    • Minor refactor in evaluations.rs and mod.rs to streamline evaluation score saving.

This description was created by Ellipsis for a87b224. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to a87b224 in 1 minute and 27 seconds

More details
  • Looked at 4965 lines of code in 22 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:66
  • Draft comment:
    The label naming convention should be updated to label_class_name (username) as mentioned in the PR description. Please ensure consistency across the codebase.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/components/queue/labels.tsx:126
  • Draft comment:
    Consider refactoring the sorting logic for label values into a shared utility function to avoid code duplication. This logic is also present in frontend/components/traces/add-label-popover.tsx.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR description mentions ordering label values by their numeric score value when displaying. This is implemented in frontend/components/queue/labels.ts and frontend/components/traces/add-label-popover.tsx. However, the sorting logic is duplicated. It would be better to refactor this into a shared utility function to avoid code duplication.
3. app-server/src/db/evaluations.rs:148
  • Draft comment:
    Consider adding a feature flag check similar to the one in app-server/src/ch/evaluation_scores.rs to ensure the function returns early if the feature is not enabled.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, suggesting a feature flag check without strong evidence that it is needed. The function add_evaluation_score is straightforward, inserting a score into the database, and there is no indication that a feature flag is relevant here. Without more context on why a feature flag would be necessary, the comment seems unnecessary.
    I might be missing the broader context of the application, where feature flags are commonly used. However, without specific evidence or reasoning provided in the comment, it remains speculative.
    Even if feature flags are used elsewhere, the comment does not provide a concrete reason or evidence for needing one here, making it speculative.
    The comment is speculative and does not provide strong evidence for a necessary code change. It should be deleted.
4. app-server/src/ch/evaluation_scores.rs:276
  • Draft comment:
    Consider adding a feature flag check similar to the one in app-server/src/db/evaluations.rs to ensure the function returns early if the feature is not enabled.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_rb6tDKBuyF3z5eIT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@dinmukhamedm dinmukhamedm merged commit aad5b79 into dev Nov 26, 2024
1 check passed
@dinmukhamedm dinmukhamedm deleted the label-eval-link branch November 26, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant