Add Agent Builder and Evaluators API clients - #28
Conversation
837c4d8 to
24acce2
Compare
milistu
left a comment
There was a problem hiding this comment.
@mafaldasv-dev Very good job on the PR! 🤗
I left a few blocking comments and some suggestions. Happy to discuss if anything is unclear.
thought (non-blocking, process): one thing for future PRs rather than this one is that this bundles two issues that could have been separate PRs. The evaluators work stands completely on its own, and the Agent Builder client could have followed as its own PR, even stacked on top of the first branch if it depended on it. Splitting would have made both halves easier to review, and it lets the independent half merge while the other is still in discussion.
suggestion (architecture, blocking): Should AgentBuilderClient live under api/ with other Kibana clients?
My understanding is that this SD is meant to be a general evals framework (the Python equivalent to kbn-evals). I imagine that Agent Builder will be a heavy user of it, but so will other teams, and some of them won't touch Agent Builder at all. Additionally, kb-evals itself does not know anything about Agent Builder, that setup lives in suite-specific code.
Since AgentBuilderClient is a Kibana HTTP client just like KibanaScoresClient, KibanaDatasetClient and KibanaEvaluatorsClient, I think it belongs next to them:
src/elastic_evals/api/
agent_builder_client.py # example
datasets_client.py
evaluators_client.py
...This would also let it reuse the shared infrastructure in api/ (like retry.py, errors.py, headers.py, response.py) instead of the parallel copies it currently carries, which breaks DRY methodology. I have separate comments on those duplications, but the move would make most of them resolve naturally.
I would appreciate your thoughts on this, and I am happy to discuss it if there is a reason for the top-level placement that I'm missing.
suggestion (test, blocking): the biggest gap I see is that the four trace evaluators have no unit tests at all. The acceptance tests cover the KibanaEvaluatorsClient HTTP workflow, but not the evaluator logic itself. I would consider this one blocking.
There was a problem hiding this comment.
Thank you for this PR, great work @mafaldasv-dev 🎊 , I also left a few comments and suggestions !
suggestion (architecture): The "parse body, build message, raise typed error"block is copy-pasted across all four clients now:
KibanaDatasetsClient (_raise_dataset_sync_error)KibanaScoresClient (inline in ingest_scores)KibanaEvaluatorsClient (inside _request)AgentBuilderClient(_raise_error, which also re-implementsparse_error_bodyinline)
They only differ in the message prefix and the error class. What if we pull it into api/response.py next to parse_error_body?
def raise_kibana_error(response, *, error_cls, context) -> NoReturn:
body, body_text = parse_error_body(response)
message = f"{context} failed with {response.status_code}"
if body_text:
message = f"{message}: {body_text}"
raise error_cls(message=message, status_code=response.status_code,
body=body, retryable=is_retryable_status_code(response.status_code))Each client then becomes a one-liner like
raise_kibana_error(response, error_cls=KibanaEvaluatorsError, context="Kibana evaluators request").
There was a problem hiding this comment.
@mafaldasv-dev I have a general question about the evaluator approach.
Why are we re-implementing the evaluators in Python? All the built-in evaluators in kbn-evals can be invoked via the POST /internal/evals/_evaluate API.
Example:
POST /internal/evals/_evaluate
{
"evaluators": [{ "name": "input_tokens" }]
....
}
The KibanaEvaluatorsClient you added here is the right direction, and we shouldn't re-implement the built-in evaluators (latency, input_tokens, output_tokens, tool_calls) again here.
[suggestion]
Replace the evaluator packages with one factory that maps evaluator names to Evaluator objects backed by KibanaEvaluatorsClient.evaluate() (you can batch all evaluators for a given trace into a single request). And then consume it with something like:
evaluators = kibana_evaluators(
[
EvaluatorConfig(name="input_tokens", kind="CODE"),
EvaluatorConfig(name="output_tokens", kind="CODE"),
EvaluatorConfig(name="latency", kind="CODE"),
EvaluatorConfig(name="tool_calls", kind="CODE"),
],
client=evaluators_client,
)Note: correctness and groundedness are also in Kibana's registry and could move to the same path. They were probably added here before the _evaluate API was available in Kibana.
Does that make sense? Or am I missing something?
I am also thinking in the same direction as @viduni94. Generally speaking my questions are:
But overall very nice work! 🚀 |
|
@milistu thank you for your careful review!
I'm gonna be honest: the only reason why I didn't separate this PR into 2 was to avoid more overhead of having the other open PR (with the PoC scripts) being rebased on two PRs 😅 in normal circumstances yes, I would separate them and try to have smaller PRs. I'll keep this in mind for the future.
My initial thoughts regarding this were that agent builder is more a utility rather than something we want to make a deep effort to mirror and maintain. Contrary to the other clients, this python sdk doesn't need this client to continue to exist and to offer the existent evals framework's features, so I kinda wanted to separate this from the rest. It comes with the downfall you rightfully pointed: it makes harder to reuse some of the code around retries, errors, etc (or if I wanted to reuse them, there would be weird imports and internal dependencies happening). With this said, I'm not too attached with the idea of keeping agent builder client as a separate module. I will leave to you the final decision on whether you want me to do this refactoring (move agent builder client to apis) or if you are ok with my reasoning.
I forgot about them, nice catch! Will add the tests shortly. |
|
@viduni94 & @avillalba-elastic (tagging Ana as well as she had a similar comment :))
Regarding your question about implementing the evaluators directly instead of calling directly the API - I was simply following the existent pattern in this SDK. However, I agree with your points, and calling directly the API will avoid potential drift problems. The only question that I have is if you guys mind if I do this refactoring in a separate PR (with a new issue) or do you really want the refactoring happening already in this PR? I'll leave that decision to you. 🙂 |
|
Hi, @avillalba-elastic !
About your questions: |
@mafaldasv-dev I'm okay with a follow up PR, but let's remove the 4 new evaluators from this PR in that case and add that in the follow up PR using the APIs. |
@viduni94 I changed my mind and did the refactoring as you suggested. :) |
@MinasCham I've implemented your suggestion, thanks! |
I'll then review the other PR and make any comment there. This LGTM but I'll let your teammates decide when to approve! |
5f2664d to
d218703
Compare
viduni94
left a comment
There was a problem hiding this comment.
The evaluator changes to use the Kibana APIs LGTM
I didn't do a deep code review, will leave that for @milistu and @MinasCham
Overall, amazing work @mafaldasv-dev! 🎉
Thank you
milistu
left a comment
There was a problem hiding this comment.
This is a good progression, really nice work @mafaldasv-dev 🤗
suggestion(non-blocking): this approach is going in a good direction. I would just suggest a minor improvement.
If I understand correctly, the Agent Builder API is integrated for a better user experience, so users do not need to reimplement the same setup code over and over. We don't own that API, we just use it, and I agree that's a distinction worth encoding in the structure.
But I think _internal doesn't express that distinction. What it says is "users must not touch this", which contradicts the purpose of the client. That is also why the # noqa: PLC2701 suppression is needed, the linter is warning about importing a private module from a package.
I suggest renaming the _internal to integrations. That keeps the separation we want (our APIs in api/, external APIs we consume in integrations/) while leaving the client public for the users it was built for.
The example in examples/agent_builder/tasks/agent_builder.py still calls the endpoint with raw httpx instead of this client. If the client were public and promoted, that example would be its first customer :D
Your call whether to include it.
MinasCham
left a comment
There was a problem hiding this comment.
Thank you for addressing the comments @mafaldasv-dev , changes LGTM, just left a question more as a food for thought
| self._client = client | ||
| self._configs = tuple(configs) | ||
| self._instrumentation_profile = instrumentation_profile | ||
| self._evaluations: dict[str, asyncio.Task[EvaluateResponse]] = {} |
There was a problem hiding this comment.
question (non-blocking): How should this cache behave if the same trace gets evaluated again with different reference data (or if the first request fails)?
Since it is keyed only by trace_id, both cases reuse the original task, including its result or exception. More of a food-for-thought question whether these evaluators are intended to be reusable across multiple runs.
There was a problem hiding this comment.
@milistu and @MinasCham to address both of your comments :)
I added combined correctness and groundedness factories so qualitative and quantitative results share one Kibana request per evaluation. The analysis creators now accept KibanaEvaluatorsClient and connector_id directly, while keeping the old inference-client usage compatible.
I also scoped the cache to a single evaluation, included the full request data in its identity, and removed failed requests from the cache. Therefore, reusing a trace with different reference data (or in a new evaluation, for example) also makes a fresh request.
697c9b1 to
f9ff334
Compare
Rebased on [PR #28 (Add Agent Builder and Evaluators API client)](#28) Closes [#5846](elastic/observability-dev#5846). ## Summary This PR implements the Agent Builder evaluation PoC used to compare the existing Opik workflow with the Python SDK backed by `kbn/evals`. It demonstrates two ways of conducting the same WixQA evaluation use case: ### Managed workflow `run.py` uses `ElasticEvalsClient.run_experiment()` to manage task execution, SDK-side evaluators, custom evaluation, tracing, and score ingestion. ### Granular workflow `run2.py` performs the workflow without `run_experiment()`. It directly coordinates: - Kibana dataset synchronization. - Agent Builder setup and Converse calls. - Evaluator discovery, validation, and execution. - Score ingestion. - Custom Document Recall scoring. - Importing external Precision@K, Recall@K, and F1@K scores from Orca. The PoC can load the public `Wix/WixQA` dataset and knowledge base from Hugging Face or use the internal GCS source. Both scripts allow users to select a small sample or run the complete source dataset. The PR also includes setup documentation, environment templates, Elasticsearch indexing helpers, retained Opik comparison utilities, and mocked tests for the data and evaluation workflows. ## Testing - Pre-commit hooks pass. - Full test suite passes with the PoC dependencies installed: ``` uv sync --locked uv run --no-sync pytest tests/ ``` ## Risks The PoC depends on local Elasticsearch, Kibana, EDOT, configured inference connectors, and Agent Builder. Dataset synchronization replaces previous dataset contents, and the Elasticsearch index is recreated on every execution. This PR must be merged after #28 because it uses the Agent Builder and Evaluators API clients introduced there.
Closes #5902.
Closes #5903.
Summary
Adds the SDK functionality required to run Agent Builder evaluations from Python:
These additions allow Python workflows to configure Agent Builder, execute conversations, evaluate their traces, and ingest the resulting scores through the existing evals infrastructure.
Risks
Kibana’s Agent Builder and Evaluators APIs are still evolving, so their request and response contracts may require follow-up adjustments.