Repository navigation
Conversation
Summary of ChangesHello @ftnext, 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 enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 effectively adds support for custom metrics within the AgentEvaluator, which is a valuable enhancement for pytest-based evaluations. The changes are well-implemented, including the refactoring of get_default_metric_info into its own module for better separation of concerns and fixing a critical issue in MetricEvaluatorRegistry by changing the registry from a class to an instance attribute. The addition of both unit and integration tests ensures the new functionality is robust. I have a minor suggestion to improve code style by relocating an inline import.
rohityan
added
the
request clarification
|
Hi @ftnext , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please address the suggestions. |
|
@ryanaiagent Thank you for the review! I've addressed the suggestions. |
|
To "request clarification" label: |
|
Hi @rohityan — any update on the timeline for merging this? We're hitting this exact issue when running custom metrics via AgentEvaluator.evaluate() in pytest. Would be happy to help test or address remaining review feedback if that helps move things forward. |
rohityan
removed
the
request clarification
|
Hi @ankursharmas , can you please review this. |
Use a per-evaluation metric registry populated from EvalConfig so pytest-based agent evaluations resolve custom metrics consistently with the CLI flow.\n\nMerge the latest main branch and adapt the implementation to the shared registry helper.\n\nCloses google#4343
|
Resolved the merge conflicts with the latest main and adapted the change to the current metric registry implementation. Unit tests pass (68 passed). |
Merge #4344 **Please ensure you have read the [contribution guide](https://github.com/google/adk-python/blob/main/CONTRIBUTING.md) before creating a pull request.** ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Closes: #4343 **Problem:** `AgentEvaluator.evaluate` did not register custom metrics from `EvalConfig`, so custom metrics worked in `adk eval` but not in pytest-based evals. **Solution:** Align `AgentEvaluator` with the CLI eval flow by registering custom metrics via a per-run metric registry and a shared default `MetricInfo` helper. The per-run registry is a fork of `DEFAULT_METRIC_EVALUATOR_REGISTRY` (new `MetricEvaluatorRegistry.fork()`), so the custom metrics declared by one eval config never leak into another run. ### Testing Plan Add unit coverage for the registration behavior and a lightweight integration example that uses a custom metric. **Unit Tests:** - [x] I have added or updated unit tests for my change. - [x] All unit tests pass locally. ``` % pytest tests/unittests/evaluation tests/unittests/cli 614 passed, 308 warnings in 12.74s ``` **Manual End-to-End (E2E) Tests:** ``` % pytest tests/integration/test_with_test_file.py::test_with_custom_metric tests/integration/test_with_test_file.py . [100%] 1 passed, 12 warnings in 3.57s ``` ### Checklist - [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-python/blob/main/CONTRIBUTING.md) document. - [x] I have performed a self-review of my own code. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have added tests that prove my fix is effective or that my feature works. - [x] New and existing unit tests pass locally with my changes. - [x] I have manually tested my changes end-to-end. - [x] Any dependent changes have been merged and published in downstream modules. ### Additional context This change keeps `AgentEvaluator` behavior consistent with `adk eval` while avoiding CLI-layer dependencies. `fork()` returns an isolated copy seeded with the source registry's contents, rather than a bare `MetricEvaluatorRegistry()`. The seeding is what keeps this backwards compatible: registering an `Evaluator` subclass on `DEFAULT_METRIC_EVALUATOR_REGISTRY` is the only way to plug one in, since an eval config can only name a scoring function. Callers who do that today (including ones replacing the evaluator behind a standard metric name) would otherwise silently fall back to the stock evaluator, with no error and a different score. Covered by `test_evaluate_eval_set_keeps_evaluators_from_the_default_registry`. Co-authored-by: Yi Liu <yiliuly@google.com> COPYBARA_INTEGRATE_REVIEW=#4344 from ftnext:agent-evaluator-support-custom-metric 3c844a9 PiperOrigin-RevId: 966416454
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
AgentEvaluator.evaluatedid not register custom metrics fromEvalConfig, so custom metrics worked inadk evalbut not in pytest-based evals.Solution:
Align
AgentEvaluatorwith the CLI eval flow by registering custom metrics via a local metric registry and a shared defaultMetricInfohelper.Testing Plan
Add unit coverage for the registration behavior and a lightweight integration example that uses a custom metric.
Unit Tests:
Manual End-to-End (E2E) Tests:
Checklist
Additional context
This change keeps
AgentEvaluatorbehavior consistent withadk evalwhile avoiding CLI-layer dependencies.