feat(review): human review queues for low-score, high-latency, and high-cost traces (#2700) - #2723
feat(review): human review queues for low-score, high-latency, and high-cost traces (#2700)#2723Payal2000 wants to merge 1 commit into
Conversation
…h-cost traces Adds an opinionated, dashboard-based human review workflow with a fixed review record and pull-based queues. All state lives in the configured database and all work runs in the active SDK or dashboard process. Review record and queues: - `HumanReview` with the fixed field set: verdict, score, failure type, corrected output, notes and reviewer. Verdict is required; score is bounded to [0.0, 1.0]; verdict and failure type are closed enums validated before anything is persisted. - `ReviewQueue`, `ReviewItem` and `ReviewTarget`, in new tables added by an additive alembic revision 13. - Item ids derive from the queue and the target, so adding the same target to a queue twice is idempotent and membership stays stable. Selection is DataFrame-first, over what `get_records_and_feedback()` already returns: - `low_score`/`high_score`/`worst_score`, `high_latency`/`slowest`, `high_cost`/`most_expensive`, `has_error`, composable with `&` and `|`. - `worst_score` respects the direction reported with the records, defaulting to higher-is-better with a warning when none is reported. - Latency and cost read `latency`, `total_cost` and `cost_currency` without estimating missing values; cost predicates require a currency so amounts in different currencies are never compared. - Each target freezes its selection reason, the relevant metric/latency/cost values, app name and version, and a normalized priority for ordering, so recomputing source metrics never changes what a reviewer sees. - `ReviewTargets.preview()` runs the same selection as `from_records()`, so previewed ids are exactly the ids materialized into a queue. Queue behaviour: - Claiming performs one conditional update from pending to in_review, so two callers racing for an item cannot both succeed. - Stale claims are recovered only when another caller asks for work; there is no background reaper. - Items can be skipped, released, or marked unavailable, and a missing source target stays visible in the queue rather than disappearing. - Editing a review writes a superseding row rather than overwriting, so history is preserved and reviewer labels review independently. - Direct reviews need no queue and use the same persistence model. - Deleting a queue leaves its reviews and the source traces alone. Dashboard: a Review page with queue-creation presets over the loaded records (low score, high latency, high cost), preview before materializing, pull and review with the frozen selection reason shown, queue progress, and CSV/JSON export. Records and Compare can add selected rows to a queue. Adds 108 tests covering enum and score-range validation, both metric directions, the latency and cost presets, currency separation, missing/NaN values, predicate composition, severity ordering and limits, preview parity, supersession and multiple reviewers, queue idempotency, every item-state transition, concurrent claims, stale-claim recovery, and the dashboard flows. Concurrent claiming is exercised against SQLite in unit tests and against both SQLite and PostgreSQL in the integration suite. Closes truera#2700
|
@Payal2000 Sorry for the slow reply, and nice work on this! Happy to take the manual dashboard pass. I have the repo and a working environment from #2708, so I can run the Review page end to end, work a queue through claim, skip, release and supersede, and report anything that breaks. I will post findings in this PR. One thing worth checking before you work around item 3: |
joshreini1
left a comment
There was a problem hiding this comment.
Excellent work on this feature. The architecture is clean, concurrency handling is correct (verified by real race tests), and test coverage is comprehensive. Two questions before merge: (1) The PR notes manual dashboard testing is pending—@Abelo9996 volunteered to run the Review page end-to-end; should we wait for that feedback? (2) The discussion mentions EVAL_ROOT.HIGHER_IS_BETTER was added in #1998 but there's a TODO about it—should worst_score read that attribute when present instead of always warning? No blocking issues. VERDICT: Comment—awaiting manual dashboard verification and direction-attribute clarification.
| review_queue_id = Column( | ||
| TYPE_ID, | ||
| ForeignKey(f"{prefix}review_queues.review_queue_id"), | ||
| nullable=False, |
There was a problem hiding this comment.
The comment # See NOTE(backref order_by). references a note that doesn't exist in the file. Either add the note explaining why backref ordering is used here, or remove the comment if it's self-evident.
|
|
||
| _MAX_CLAIM_ATTEMPTS = 8 | ||
| """How many times a claim retries after losing a race for a candidate item.""" | ||
|
|
There was a problem hiding this comment.
The docstring says priority is "authoritative for anything that changes," but priority is frozen at selection time and never changes after item creation. Consider rewording to "The scalar columns are authoritative for mutable state—claim token, claimed_at, and item state—because those are what the conditional UPDATE writes."
|
|
||
|
|
||
| def _resolved_direction(records: pd.DataFrame, metric: str) -> bool: | ||
| """A metric's direction, defaulting to higher-is-better with a warning.""" |
There was a problem hiding this comment.
When no direction column exists, this defaults to higher-is-better with a warning. Per the PR discussion, EVAL_ROOT.HIGHER_IS_BETTER was added in #1998. Should this function check for that span attribute first before warning, so current eval results never warn?
| @@ -0,0 +1,493 @@ | |||
| """Human review queues. | |||
There was a problem hiding this comment.
The PR description notes "I have not clicked through a live dashboard, so a manual pass there is worth doing before merge." @Abelo9996 volunteered to test claim/skip/release/supersede flows. Manual verification is blocking but I don't care if you do it or @Abelo9996 - in either case, please take a quick screencapture of the demo and share in the PR description.
Did the manual dashboard pass on this. I ran the Review page end to end against a local SQLite DB (OTEL tracing on, a What works live: claim → skip → release → re-claim → submit all behave correctly, the frozen selection reason renders ("Queued because: Groundedness < 0.5" with the metric/priority snapshot), progress counters update, and the Export tab produces the completed-reviews table + CSV/JSON. I hit two things that only show up in a real browser (both slipped past the 1. Trace panel crashes on every claim under OTEL. 2. The "Record a score" toggle can never enable the slider. The toggle and the Both fixes (verified live: Trace panel now renders spans, slider enables): with st.expander("Trace", expanded=False):
if is_otel_tracing_enabled():
- events = _get_event_otel_spans(record_ids=[target_id])
- if events is not None and not events.empty:
- record_viewer_otel(events, key=f"review_{target_id}")
+ events = _get_event_otel_spans(target_id, row.get("app_name"))
+ if events:
+ record_viewer_otel(spans=events, key=f"review_{target_id}")
else:
st.caption("No trace data available for this record.") session = get_session()
+ # The score toggle must live outside the form: widgets inside an
+ # st.form do not trigger a rerun until submit, so a slider whose
+ # `disabled` state depends on an in-form toggle can never be enabled.
+ set_score = st.toggle("Record a score")
+
with st.form(f"{page_name}.review_form", border=True):
...
cols = st_columns(2)
with cols[0]:
- set_score = st.toggle("Record a score")
score = st.slider(
"Score", 0.0, 1.0, 0.5, 0.05, disabled=not set_score
)One gap, not a bug: supersession works at the API level (a second Happy to open these as a small PR against your branch if that's easier than pasting the diffs. |
Here's how the dashboard functions with my aforementioned fixes included: Screen.Recording.2026-08-26.at.1.40.44.PM.mov |
|
Thank you @Payal2000 and @Abelo9996 - this is super useful and great to see the UI workflow. The difference between release and skip is unclear to me, can you clarify? |
|
They differ by the state the item lands in:
One thing I noticed doing the live pass, worth a look: for a single reviewer, Release then Pull-next returns the same item. That's fine in a multi-reviewer setting where another caller grabs it, but in a solo session Release doesn't visibly "set the item aside." Is the loop intended? |
joshreini1
left a comment
There was a problem hiding this comment.
Really good progress here. The changes below are about making the annotation queue usable by more than one reviewer, and I think they make a large difference to the impact of the feature.
The selection layer is the part I would not touch: direction-aware worst_score, required currency on cost predicates, NaN never matching, preview() sharing one code path with from_records(), and the frozen SelectionSnapshot. The conditional-update claim with a rowcount check is the right primitive. Content-addressed supersession is right. The additive migration and the 108 tests are right.
There are two root problems: where state lives, and what a reviewer is. The other three issues follow from them.
1. A queue can only be worked by one reviewer
#2700 asks for "multiple reviewer labels may independently review the same target". That works for direct off-queue reviews but not through a queue.
state, claim_token, claimed_at, claimed_by and current_review_id are all on ReviewItem, and review_item_id is derived from (review_queue_id, target_type, target_id) (schema/review.py:371). So one target in one queue is one claimable row with one state. The first submit sets COMPLETED, and the claim query only considers PENDING or stale IN_REVIEW (sqlalchemy.py:2373), so no second reviewer can be handed that target. current_review_id is also a single pointer, so the item could not reference a second review even if one existed.
The workarounds are N parallel queues (duplicated selection, N separate progress counters, the same trace under different item ids) or direct reviews with no queue (no claiming, no progress, no queue attribution).
2. The reviewer label is required for correctness but left optional
TruLens has no users or accounts, and #2700 lists "authentication or verified reviewer identity" as a non-goal. A free-text reviewer string, and claimed_by alongside it, is the right call. The problem is that correctness now depends on that string while it stays optional, unnormalized, and typed per submission.
Supersession is keyed on it. At submit (session.py:1439):
previous = self.connector.db.get_latest_human_review(
target_type=target_type, target_id=target_id, reviewer=reviewer
)and in the lookup (sqlalchemy.py:2536):
if reviewer is not None:
query = query.filter_by(reviewer=reviewer)reviewer=None means "the latest review by anyone" rather than "the anonymous reviewer's latest review". The dashboard's Reviewer box is optional and empty by default (reviewer=reviewer or None), so if two people leave it blank, the second submit sets supersedes_id to the first person's review and hides it. That is a bug in the PR as written, separate from anything below.
The dashboard also never sets a reviewer before pulling. claim_next_review_item(review_queue_id) is called with no reviewer (Review.py:380), so claimed_by is always None there, and skip and release are anonymous.
3. SKIPPED means two things and resolves both destructively
SKIPPED is in TERMINAL_ITEM_STATES, so session.skip_review_item(item) removes the item for everyone, with no reviewer, no note, and no API path back. It is being used for two unrelated cases:
- "This trace cannot be judged" (truncated, empty output, source gone). That is a finding about the target and should be attributed and exported.
- "Not mine" (routing). That should apply to one reviewer only.
#2700 lists skip as an action but does not define its scope. Making it global and anonymous contradicts the multiple-reviewers requirement. It also hurts the solo case: a single annotator who skips to come back later has no supported way to get the item back.
4. release re-serves the same item to the same reviewer
release_review_item sets PENDING and clears the claim. Severity ordering is deterministic and stable, so the next claim_next_review_item returns the same item. For one reviewer working alone, release loops. Skip and Release also sit next to each other in the UI with nothing explaining when to use which.
5. Stale-claim takeover does not handle the loser
insert_human_review does not check a claim (sqlalchemy.py:2498), but update_review_item_state rejects a mismatched token (sqlalchemy.py:2473). So a reviewer whose claim aged out and was taken over can still submit: the HumanReview row is written and the item transition raises, leaving the review detached from an item someone else has completed. Attaching reviews to per-reviewer assignments makes this harmless, but it needs a test either way.
What the PR should change
A. Move state onto a per-reviewer assignment
ReviewItem becomes target membership. A new ReviewAssignment holds state and the claim.
graph TD
Queue["ReviewQueue: reviews_required, reviewers"] --> Item["ReviewItem: target, priority, selection, ACTIVE or UNAVAILABLE"]
Item --> A0["ReviewAssignment slot 0: state, claim_token, claimed_by, review_id"]
Item --> A1["ReviewAssignment slot 1: state, claim_token, claimed_by, review_id"]
A0 --> R0["HumanReview by josh"]
A1 --> R1["HumanReview by payal"]
Item --> D["ReviewDecline: item, reviewer"]
ReviewQueuegainsreviews_required: int = 1, validated>= 1.ReviewItemkeepsreview_queue_id, target,priority,selection, and a state ofACTIVEorUNAVAILABLE.UNAVAILABLEis unchanged from today. It is a fact about the target that applies to every reviewer, so it belongs on the item rather than the assignment. Item id derivation is unchanged, so idempotent re-add is unchanged.ReviewAssignmenthasreview_assignment_id = hash(review_item_id, slot),stateinPENDING | IN_REVIEW | COMPLETED,claim_token,claimed_at,claimed_by,human_review_id. Denormalizereview_queue_idandpriorityonto it so the claim stays one indexed scan plus the same conditionalUPDATEthat exists today.add_review_itemscreatesreviews_requiredassignments per target. Assignment ids are derived, so raisingreviews_requiredlater and re-provisioning only adds the new slots.ReviewDecline(review_item_id, reviewer, ts, notes)records a reviewer-scoped pass. It has to outlive the assignment, which goes back toPENDINGfor someone else, so it cannot be a field on the assignment. It must not count towardreviews_required, so it cannot be aHumanReview.
Claim becomes: assignments in the queue whose item is ACTIVE, state PENDING or stale IN_REVIEW, with NOT EXISTS a sibling assignment on the same item claimed by or reviewed by this reviewer, and NOT EXISTS a decline by this reviewer. Ordering is unchanged, and so is the rowcount-checked update.
At reviews_required=1 this behaves the same as the PR does now for a single annotator: one assignment per target, same ordering, same claim, same completion. Multi-reviewer is the same code path with a larger reviews_required, not a second mode.
B. Make the reviewer label required and normalized
No authentication, no user table, no permissions. The label stays a label, but it stops being optional and per-submission, because the per-reviewer guards in A depend on it. A blank label makes everyone one reviewer, so a reviews_required=2 queue gives both slots to the first person. A typo makes one person two reviewers, so they get the same target twice.
- Require a non-empty, trimmed
revieweronclaim,pass, and submit-against-an-assignment. Direct off-queue reviews stay optional, matching the field table in #2700 whererevieweris not required. - Add
reviewers: Optional[list[str]]toReviewQueue. When set, the dashboard shows a dropdown and claim/pass/submit reject labels outside the list. When unset, free text as today. - Set the label once per dashboard session, above the Pull button, in
st.session_state, and display it. Remove the per-form Reviewer input. - In
get_latest_human_review, matchreviewer IS NULLwhenrevieweris None instead of dropping the filter. That fixes problem 2 on its own.
This is unverified attribution: anyone can type anyone's name. That is fine for a team labeling its own traces and consistent with the non-goal, but say so in the docstrings.
C. Replace skip with three separate actions
| Action | Means | Scope | Terminal | Attributed |
|---|---|---|---|---|
submit_human_review(verdict="unreviewable") |
cannot be judged | that assignment | yes, COMPLETED |
full HumanReview |
pass_review_item(item, reviewer) |
not for me | that reviewer | no, back to PENDING |
ReviewDecline row |
mark_review_item_unavailable(item) |
source trace will not load | the target, everyone | yes | unchanged |
Add Verdict.UNREVIEWABLE, drop ReviewItemState.SKIPPED and session.skip_review_item. A skip then counts toward reviews_required and appears in the export instead of leaving a gap. For a solo reviewer, pass is "send this to the back of my queue", which is what the skip button was standing in for.
D. Reduce release to claim recovery
Release means only "I dropped my claim": closed tab, crashed process, wrong queue. No memory, no reviewer intent, and not a reviewer-facing button. Reviewers use pass instead. That removes the loop without adding cooldown state, since the action that meant "later" now has its own verb.
E. Report agreement, do not resolve it
Adjudication and consensus stay non-goals. But with N reviews per target the export is misleading without a rollup, so add get_target_reviews(queue, target) and per-target n_reviews, verdict distribution, and an agreement flag to progress and to the CSV/JSON export. Resolution can be a follow-up issue.
How this maps back to #2700
- "Multiple reviewer labels may independently review the same target" works through a queue, and does not depend on each reviewer remembering to type a distinct label.
- "Review supersession preserves complete history" holds. Today a blank label supersedes someone else's review.
- "Reviewers can skip, release, or complete an item" is kept, with skip's scope defined so it stops contradicting the bullet above.
- "Missing source targets remain visible and can be skipped or marked unavailable" is kept.
UNAVAILABLEis unchanged, and skip becomes a verdict that leaves the item visible with a reason. - Authentication stays out. A required label is not an identity: nothing is verified, there is no user table, and the roster is a typo guard, not a permission list.
- Push assignment, permissions, consensus and adjudication stay out. Work is still pulled and nothing is assigned to anyone.
- No new runtime. No daemon, reaper or background worker, and stale recovery still only happens on an explicit pull.
- Selection, snapshot freezing, preview parity and the conditional claim are unchanged.
Suggested order
- Fix the supersession filter and require a normalized reviewer on the queued paths. Small, standalone, and a correctness fix on its own.
- Split
ReviewIteminto membership plusReviewAssignment, addreviews_requiredand slot provisioning, rewrite the claim over assignments. - Add
Verdict.UNREVIEWABLEandpass_review_item, dropSKIPPED, reduce release to claim recovery. - Add the roster and the per-target agreement rollup in progress and export.
- Dashboard: reviewer before Pull, roster dropdown, Pass instead of Skip, no Release,
reviews_requiredon queue creation. - Fold all of it into revision
13rather than stacking a14on an unshipped schema. The PR already notes13collides with #2721, so whichever lands second renumbers anyway.
Tests worth adding
reviews_required=1 parity with current behavior; two reviewers completing one target; a reviewer never served the same target twice; pass returning work to others while excluding the passer; unreviewable counting toward completion; completion when reviews_required is reached; re-provisioning after raising reviews_required; stale takeover where the loser submits; empty and untrimmed labels rejected; off-roster label rejected; an anonymous direct review not superseding a named reviewer's. Extend the existing SQLite and PostgreSQL concurrency tests to reviews_required=2, asserting each target ends with two distinct claimed_by values and no reviewer appearing twice on one target.
Also worth running tests/integration/test_database.py with the docker test database up, since that path currently skips, and doing the manual dashboard pass the PR flags as outstanding: two browser sessions, different labels, one reviews_required=2 queue.
Closes #2700
Problem
TruLens can store a numeric human feedback result, but it cannot collect a consistent review decision, correction, or reviewer note, or work systematically through a fixed set of traces. Review tooling has to be built outside TruLens even though the Records page already has the trace context a reviewer needs.
What this adds
One opinionated, dashboard-based human review workflow with a fixed review record and pull-based queues. All state lives in the configured user-owned database and all work runs in the active SDK or dashboard process — no daemon, no hosted service, no background worker.
Review record and queues
HumanReviewwith the closed field set from the issue:verdict(required),score,failure_type,corrected_output,notes,reviewer. Verdict and failure type are fixed enums and score is bounded to[0.0, 1.0], all validated before anything is persisted.ReviewQueue,ReviewItemandReviewTarget, in three new tables added by an additive alembic revision13.Selection
DataFrame-first over what
get_records_and_feedback()already returns, rather than a database query language:&and|.worst_scorerespects the direction reported alongside the records, so "worst" is the lowest scores for a higher-is-better metric and the highest for a lower-is-better one. When the records carry no direction it defaults to higher-is-better and says so in a warning.latency,total_costandcost_currencyand never estimate a missing value. Cost predicates take a requiredcurrencyso amounts in different currencies are never compared — a EUR row is simply not eligible for a USD threshold.ReviewTargets.preview()runs the identical selection asfrom_records(), so previewed ids are exactly the ids materialized into the queue.Queue behavior
pendingtoin_reviewand checks the rowcount, so two callers racing for the same item cannot both succeed.Dashboard
A Review page with a queue-creation panel offering the three presets over the loaded records (low evaluation score, high latency, high cost), threshold or top-N, preview then materialize, pull-and-review with the frozen selection reason displayed, queue progress and state counts, and CSV/JSON export from the active process. Records and Compare can add selected rows to a queue.
Tests
108 tests covering every bullet on the issue's test list: enum and score-range validation, low/worst score in both metric directions, latency and cost presets, currency separation, errors, predicate composition, severity ordering and limits, missing metric / missing cost / NaN score / mixed-currency behavior, selection snapshot and preview-materialization parity, supersession and multiple reviewers, queue idempotency and every item-state transition, stale-claim recovery, missing targets, dashboard create/add/pull/review/export flows, and additive-migration and database-reset behavior.
Concurrent claiming is genuinely exercised rather than asserted: four threads race over ten items through separate database connections and the test checks each item was claimed exactly once. That runs against SQLite in the unit suite and against both SQLite and PostgreSQL in
tests/integration/test_database.py.Verified locally: the non-OTEL unit suite goes from 396 to 503 passing with no new failures — the remaining failures are byte-identical on a clean tree. Formatting and lint are clean under the ruff version pinned in
.pre-commit-config.yaml. The diff is purely additive: no existing file loses a line.Notes for review
DBare concrete stubs that raiseNotImplementedError, not@abc.abstractmethod. There is a second implementation,SnowflakeEventTableDB, that abstract methods would have broken.ReviewItemkeeps state, priority and the claim in scalar columns rather than the JSON blob, with the blob holding only the frozen selection snapshot. That separation is what allows the claim to be a single conditionalUPDATE.AppTestplus the session calls each flow makes. I have not clicked through a live dashboard, so a manual pass there is worth doing before merge.13is also claimed by feat(datasets): immutable DatasetVersion snapshots and Run provenance (#2701) #2721 (dataset versions). Whichever merges second will need its revision renumbered.