feat: evaluate testing #390
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 9c7af53 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
Trivy scanning results. Report Summary ┌─────────┬──────┬─────────────────┬─────────┐
For OSS Maintainers: VEX NoticeIf you're an OSS maintainer and Trivy has detected vulnerabilities in your project that you believe are not actually exploitable, consider issuing a VEX (Vulnerability Exploitability eXchange) statement. To disable this notice, set the TRIVY_DISABLE_VEX_NOTICE environment variable. uv.lock (uv)Total: 19 (MEDIUM: 10, HIGH: 8, CRITICAL: 1) ┌──────────────────┬────────────────┬──────────┬────────┬───────────────────┬───────────────┬──────────────────────────────────────────────────────────────┐ |
| Compute the evaluation results for the given pipeline and data. | ||
|
|
||
| Args: | ||
| pipeline: The pipeline to be evaluated. |
There was a problem hiding this comment.
I don't see an argument pipeline
|
|
||
| class EvaluationPipeline(Generic[EvaluationTargetT], WithConstructionConfig, ABC): | ||
| """ | ||
| Collection evaluation pipeline. | ||
| """ | ||
|
|
||
| CONCURRENCY: int = 10 |
There was a problem hiding this comment.
Wouldn't it be better to have this as a part of EvaluationConfig so it could be changed if needed?
There was a problem hiding this comment.
good point. also renamed the parameter to batch_size - should be more intuitive. probably we should also support multiple batching strategies here as we do withing document processing
| """ | ||
| model = EvaluationConfig.model_validate(config) | ||
| dataloader: DataLoader = DataLoader.subclass_from_config(model.dataloader) | ||
| pipeline: EvaluationPipeline = EvaluationPipeline.subclass_from_config(model.pipeline) |
There was a problem hiding this comment.
After checking the code, I am no longer sure that moving Evaluator to EvaluationPipeline is a good move. Here you are creating an EvaluationPipeline object inside EvaluationPipeline. This is confusing for the user and mixes two different abstractions that imho should be separated - the code being evaluated and the evaluation orchestration. I think we should undo this change and bring back Evaluator.
There was a problem hiding this comment.
changed it to be a class method now. would not say it is confusing - as well one can say that all class methods are confusing. To me stateless object is more confusing than this and actually the code which is being evaluated is the object evaluation_target (eg. DocumentSearch) and EvaluationPipeline is already kind of orchestration for eval execution
kdziedzic68
force-pushed
the
feat/evaluate-testing
branch
from
4957491 to
175c3eb
Compare
No description provided.