-
Notifications
You must be signed in to change notification settings - Fork 80
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
python[patch]: accept simple evaluators #1200
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.
Nice i like the start will re-review this morning
if not ( | ||
num_positional in (2, 3) or (num_positional <= 3 and has_positional_var) | ||
): | ||
msg = "" |
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
Co-authored-by: William FH <[email protected]>
…i/langsmith-sdk into bagatur/rfc_simple_evaluator
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to add like a couple unit tests on this to make it obvious it's working
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.
I think this makes sense, but would add some extra tests in to confirm it works properly
]: | ||
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add debug logs letting you know what function type is being used? Might be helpful since we tell people to enable debug logs for debugging issues in the SDK?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link feels like it's not gonna have a long shelf-life
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooc, why do we require at least one positional one?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -87,6 +87,7 @@ | |||
[schemas.Run, Optional[schemas.Example]], | |||
Union[EvaluationResult, EvaluationResults], | |||
], | |||
Callable[..., Union[dict, EvaluationResults, EvaluationResult]], |
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.
Could we update the docstring for evaluate() and aevaluate()
to have examples or link to a docs page that shows the valid arguments?
): | ||
msg = ( | ||
f"Invalid evaluator function. Must have at least one positional " | ||
f"argument. Supported positional arguments are {supported_args}. Please " |
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.
Could we include a description of what each argument is here?
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.
feels like ppl should just look at api ref for that?
can write evaluators like this:
example experiment: left-tray-86 https://dev.smith.langchain.com/public/e7782ea0-3de5-4352-8cd4-7b2cdbb03e4c/d