Aaryan/trace custom scorers #625
Conversation
Summary of ChangesHello @adivate2021, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for custom trace scorers, a significant feature enhancement. The changes correctly propagate the is_trace flag from the CLI and client methods down to the API payload, and new data models for traces are introduced. The core logic for executing trace scoring is well-structured but has some areas for improvement. My review focuses on improving the robustness, maintainability, and documentation of the new trace scoring implementation in src/judgeval/scorers/score_trace.py.
| scoring_result = generate_scoring_result( | ||
| trace.trace_spans[0], scorer_data_list, run_duration, success | ||
| ) | ||
| scoring_results[score_index] = scoring_result |
There was a problem hiding this comment.
Accessing trace.trace_spans[0] assumes that the trace_spans list is never empty. If a TraceData object is passed with an empty trace_spans list, this will raise an IndexError and crash the scoring process for that trace. It's crucial to add a check to handle this edge case gracefully to prevent runtime errors.
| scoring_result = generate_scoring_result( | |
| trace.trace_spans[0], scorer_data_list, run_duration, success | |
| ) | |
| scoring_results[score_index] = scoring_result | |
| if not trace.trace_spans: | |
| judgeval_logger.warning("Trace contains no spans. Skipping result generation for this trace.") | |
| return | |
| scoring_result = generate_scoring_result( | |
| trace.trace_spans[0], scorer_data_list, run_duration, success | |
| ) | |
| scoring_results[score_index] = scoring_result |
| scorer (TraceScorer): The `TraceScorer` to use for scoring the trace. | ||
| trace (Trace): The `Trace` to be scored. |
There was a problem hiding this comment.
The type hint for the trace parameter in the docstring is Trace, but the function signature specifies TraceData. To avoid confusion and ensure documentation accuracy, the docstring should be updated to match the signature.
| scorer (TraceScorer): The `TraceScorer` to use for scoring the trace. | |
| trace (Trace): The `Trace` to be scored. | |
| scorer (TraceScorer): The `TraceScorer` to use for scoring the trace. | |
| trace (TraceData): The `TraceData` to be scored. |
| Each `Trace` will be evaluated by all of the `TraceScorer`s in the `scorers` list. | ||
| Args: | ||
| traces (List[List[TraceSpan]]): A list of `TraceSpan` objects to be evaluated. |
There was a problem hiding this comment.
The type hint for the traces parameter in the docstring is List[List[TraceSpan]], but the function signature uses List[TraceData]. This is inconsistent and could be misleading for developers using this function. Please update the docstring to match the function signature.
| traces (List[List[TraceSpan]]): A list of `TraceSpan` objects to be evaluated. | |
| traces (List[TraceData]): A list of `TraceData` objects to be evaluated. |
| if show_progress: | ||
| with tqdm_asyncio( | ||
| desc=f"Evaluating {len(traces)} trace(s) in parallel", | ||
| unit="TraceData", | ||
| total=len(traces), | ||
| bar_format="{desc}: |{bar}|{percentage:3.0f}% ({n_fmt}/{total_fmt}) [Time Taken: {elapsed}, {rate_fmt}{postfix}]", | ||
| ) as pbar: | ||
| for i, trace in enumerate(traces): | ||
| if isinstance(trace, TraceData): | ||
| if len(scorers) == 0: | ||
| pbar.update(1) | ||
| continue | ||
|
|
||
| cloned_scorers = clone_scorers(scorers) # type: ignore | ||
| task = execute_with_semaphore( | ||
| func=a_eval_traces_helper, | ||
| scorers=cloned_scorers, | ||
| trace=trace, | ||
| scoring_results=scoring_results, | ||
| score_index=i, | ||
| ignore_errors=ignore_errors, | ||
| pbar=pbar, | ||
| ) | ||
| tasks.append(asyncio.create_task(task)) | ||
|
|
||
| await asyncio.sleep(throttle_value) | ||
| await asyncio.gather(*tasks) | ||
| else: | ||
| for i, trace in enumerate(traces): | ||
| if isinstance(trace, TraceData): | ||
| if len(scorers) == 0: | ||
| continue | ||
|
|
||
| cloned_scorers = clone_scorers(scorers) # type: ignore | ||
| task = execute_with_semaphore( | ||
| func=a_eval_traces_helper, | ||
| scorers=cloned_scorers, | ||
| trace=trace, | ||
| scoring_results=scoring_results, | ||
| score_index=i, | ||
| ignore_errors=ignore_errors, | ||
| pbar=None, | ||
| ) | ||
| tasks.append(asyncio.create_task(task)) | ||
|
|
||
| await asyncio.sleep(throttle_value) | ||
| await asyncio.gather(*tasks) |
There was a problem hiding this comment.
There is significant code duplication between the if show_progress: and else: blocks. The core logic for iterating through traces, cloning scorers, and creating asynchronous tasks is identical in both branches. This duplication makes the code harder to maintain, as any change needs to be applied in two places.
To improve maintainability, consider refactoring this to a single loop. You could use a context manager for the progress bar that does nothing when show_progress is False, allowing you to unify the logic. For example:
import contextlib
# ...
progress_context = tqdm_asyncio(...) if show_progress else contextlib.nullcontext()
with progress_context as pbar:
for i, trace in enumerate(traces):
# ... common logic for creating tasks ...
# pass pbar to helper, which can handle if it's None| Args: | ||
| scorers (List[TraceScorer]): List of TraceScorer objects to evaluate the trace. | ||
| trace (Trace): The trace to be evaluated. | ||
| scoring_results (List[TestResult]): List to store the scoring results. |
There was a problem hiding this comment.
The type hint for the scoring_results parameter in the docstring is List[TestResult]. This appears to be a copy-paste error, as the function signature correctly types it as List[ScoringResult]. Please correct the docstring to maintain consistency and avoid confusion.
| scoring_results (List[TestResult]): List to store the scoring results. | |
| scoring_results (List[ScoringResult]): List to store the scoring results. |
|
✔️ Propel has finished reviewing this change. |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| return | ||
|
|
||
|
|
||
| async def a_execute_trace_scoring( |
There was a problem hiding this comment.
[BestPractice]
[CodeDuplication] The logic in this file for scoring traces is almost a complete duplicate of the logic for scoring examples in src/judgeval/scorers/score.py. The functions safe_a_score_trace, a_execute_trace_scoring, and a_eval_traces_helper are structurally identical to their counterparts in score.py (safe_a_score_example, a_execute_scoring, a_eval_examples_helper).
To avoid code duplication and improve maintainability, consider creating a generic scoring execution utility that can be reused for both ExampleScorer with Examples and TraceScorer with TraceData.
Context for Agents
[**BestPractice**]
[CodeDuplication] The logic in this file for scoring traces is almost a complete duplicate of the logic for scoring examples in `src/judgeval/scorers/score.py`. The functions `safe_a_score_trace`, `a_execute_trace_scoring`, and `a_eval_traces_helper` are structurally identical to their counterparts in `score.py` (`safe_a_score_example`, `a_execute_scoring`, `a_eval_examples_helper`).
To avoid code duplication and improve maintainability, consider creating a generic scoring execution utility that can be reused for both `ExampleScorer` with `Example`s and `TraceScorer` with `TraceData`.
File: src/judgeval/scorers/score_trace.py
Line: 55| base_class_name = "TraceScorer" if is_trace else "ExampleScorer" | ||
|
|
||
| scorer_classes = [] | ||
| for node in ast.walk(tree): |
There was a problem hiding this comment.
[BestPractice]
The logic to find the scorer class by walking the AST has been made more complex by the introduction of the is_trace flag. This section is becoming difficult to read and maintain. Consider extracting the AST parsing and validation logic into a dedicated helper function to improve clarity and separation of concerns.
Context for Agents
[**BestPractice**]
The logic to find the scorer class by walking the AST has been made more complex by the introduction of the `is_trace` flag. This section is becoming difficult to read and maintain. Consider extracting the AST parsing and validation logic into a dedicated helper function to improve clarity and separation of concerns.
File: src/judgeval/__init__.py
Line: 130
No description provided.