-
Notifications
You must be signed in to change notification settings - Fork 20
feat: support 'same-as-agent' model option for legacy evaluators #1048
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
171a24b to
48a0c5e
Compare
Add support for the 'same-as-agent' model configuration in legacy LLM-based evaluators. When an evaluator specifies 'same-as-agent' as its model, it now resolves to the actual model from agent.json settings instead of throwing an error. Changes: - Updated EvaluatorFactory to accept and pass agent_model parameter - Added _get_agent_model() method to runtime to load model from agent.json - Added logging for model resolution and evaluator creation - Fixed error message in trajectory evaluator (was incorrectly saying "LLM evaluator") 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
48a0c5e to
9e71de8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's better to do something like this:
from typing import Protocol, runtime_checkable
@runtime_checkable
class LLMAgentProtocol(Protocol):
def get_agent_model(self) -> str:
...
And switch the implementation of _get_agent_model to:
def _get_agent_model(self, runtime: UiPathRuntimeProtocol) -> str | None:
if isinstance(runtime, LLMAgentProtocol):
return runtime.get_agent_model()
else:
return None
That way, react agent can implement that method and it should work seamlessly.
Implements the Protocol-based approach for getting agent model: - Adds LLMAgentFactoryProtocol with get_agent_model() method - Updates _get_agent_model() to check if factory implements protocol - Falls back to file-based approach if protocol not implemented This allows runtime factories to provide agent model information directly, enabling cleaner 'same-as-agent' resolution for evaluators. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Good catch implemented your Protocol-based pattern. |
|
I don't think the factory should contain the model. It's a runtime concept. I.e; the factory can create different runtimes with different LLMs based on the entrypoint, so I'd rather keep it in runtime. This is why Also, please create a unit test to test the change :) |
| cls, | ||
| data: dict[str, Any], | ||
| evaluators_dir: Path | None = None, | ||
| agent_model: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would suggest passing in agent_model_settings object instead, so that @mathurk @AAgnihotry do not have to do double work :)
|
|
||
| return result | ||
|
|
||
| def _get_agent_model(self) -> str | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when we specify a custom model settings cc - @mathurk @AAgnihotry
mathurk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
same-as-agentmodel configuration in legacy LLM-based evaluatorssame-as-agentas its model, it now resolves to the actual model fromagent.jsonsettingsChanges
EvaluatorFactory.create_evaluator()to acceptagent_modelparameter_create_legacy_evaluator_internal()to passagent_modelto LLM-based evaluators_create_legacy_llm_as_judge_evaluator()to resolvesame-as-agentto actual model_create_legacy_trajectory_evaluator()to resolvesame-as-agentto actual model_get_agent_model()method to runtime to load model fromagent.jsonTest plan
calculator_same_as_agentexample containing evaluators with"model": "same-as-agent"🤖 Generated with Claude Code