python[patch]: accept simple evaluators #1200
Conversation
Co-authored-by: William FH <[email protected]>
…i/langsmith-sdk into bagatur/rfc_simple_evaluator
baskaryan
changed the title
| @@ -632,3 +636,70 @@ def comparison_evaluator( | |||
| ) -> DynamicComparisonRunEvaluator: | |||
| """Create a comaprison evaluator from a function.""" | |||
| return DynamicComparisonRunEvaluator(func) | |||
|
|
|||
|
|
|||
| def _normalize_evaluator_func( | |||
There was a problem hiding this comment.
might be nice to add like a couple unit tests on this to make it obvious it's working
|
Do we want it to be like pytest where it's all by name? |
yea i like that. for backwards compat can't enforce run/example but can enforce the others |
| ]: | ||
| # for backwards compatibility, if args are untyped we assume they correspond to | ||
| # Run and Example: | ||
| if not (type_hints := get_type_hints(func)): |
There was a problem hiding this comment.
shouldn't we check the number of args here? traditional evaluators have run and example whereas the simple evaluators take 3 args
| if not ( | ||
| num_positional in (2, 3) or (num_positional <= 3 and has_positional_var) | ||
| ): | ||
| msg = ( | ||
| "Invalid evaluator function. Expected to take either 2 or 3 positional " | ||
| "arguments. Please see " | ||
| "https://docs.smith.langchain.com/evaluation/how_to_guides/evaluation/evaluate_llm_application#use-custom-evaluators" # noqa: E501 | ||
| ) | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
seems like this check on arg length should be moved up
| msg = ( | ||
| f"Invalid evaluator function. Must have at least one positional " | ||
| f"argument. Supported positional arguments are {supported_args}. Please " | ||
| f"see https://docs.smith.langchain.com/evaluation/how_to_guides/evaluation/evaluate_llm_application#use-custom-evaluators" |
There was a problem hiding this comment.
This link feels like it's not gonna have a long shelf-life
There was a problem hiding this comment.
ye im updating it as we speak, but there will be redirects
| if p.kind in (p.POSITIONAL_OR_KEYWORD, p.POSITIONAL_ONLY) | ||
| and p.default is p.empty | ||
| ] | ||
| if not positional_no_default or ( |
There was a problem hiding this comment.
ooc, why do we require at least one positional one?
There was a problem hiding this comment.
we only pass in the supported args as positional args, so equivalent to enforcing that there's at least one supported arg
| "outputs": run.outputs or {}, | ||
| "reference_outputs": example.outputs or {}, | ||
| } | ||
| args = (arg_map[arg] for arg in positional_no_default) |
There was a problem hiding this comment.
If I put a default in an arg this silently never provides the matching value. Would either want to validate ahead of time that no default is provided (preferred) or pass it in anyway (think not preferred)
There was a problem hiding this comment.
think im just going to remove the check for whether it has defaults or not, doesn't seem necessary
| @@ -87,6 +87,7 @@ | |||
| [schemas.Run, Optional[schemas.Example]], | |||
| Union[EvaluationResult, EvaluationResults], | |||
| ], | |||
| Callable[..., Union[dict, EvaluationResults, EvaluationResult]], | |||
There was a problem hiding this comment.
Could we update the docstring for evaluate() and aevaluate() to have examples or link to a docs page that shows the valid arguments?
There was a problem hiding this comment.
want to update docs and api ref all at once, will do as fast follow
can write evaluators like this:
example experiment: left-tray-86 https://dev.smith.langchain.com/public/e7782ea0-3de5-4352-8cd4-7b2cdbb03e4c/d