fix: trace LLMEvaluator runs #1185
Conversation
| if isinstance(self.score_config, CategoricalScoreConfig): | ||
| value = output["score"] | ||
| explanation = output.get("explanation", None) | ||
| explanation = output.get(self.reasoning_key, None) |
There was a problem hiding this comment.
I think we should handle if any of (self.reasoning_key, "reasoning", "explanation", "comment") are provided
There was a problem hiding this comment.
am slightly confused on how something other than self.score_config.reasoning_key would exist. In the _create_score_json_schema don't we only set score_config.reasoning_key? I am probably just missing something simple
| include_explanation: bool = False | ||
| explanation_description: Optional[str] = None | ||
| reasoning_key: Optional[str] = None | ||
| reasoning_description: Optional[str] = None |
There was a problem hiding this comment.
This is a breaking change, so we'll need to be able to handle accepting the other value in the init and log a deprecation warning
| if isinstance(score_config, CategoricalScoreConfig): | ||
| properties["score"] = { | ||
| properties["value"] = { | ||
| "type": "string", | ||
| "enum": score_config.choices, | ||
| "description": f"The score for the evaluation, one of " |
There was a problem hiding this comment.
It isn't a score anymore, is it? It's the selected category.
The descriptions here matter a lot
| await self.runnable.ainvoke(variables, config={"run_id": source_run_id}), | ||
| ) | ||
|
|
||
| return self._parse_output(output, str(source_run_id)) |
There was a problem hiding this comment.
nit: Could leave as a UUID
| @@ -15,7 +15,7 @@ def test_llm_evaluator_init() -> None: | |||
| key="vagueness", | |||
| choices=["Y", "N"], | |||
| description="Whether the response is vague. Y for yes, N for no.", | |||
| include_explanation=True, | |||
There was a problem hiding this comment.
Should keep a couple of these tests with include_explanation around for backward compat testing
There was a problem hiding this comment.
added some backward compatibility tests
| description (str): Detailed description provided to the LLM judge of what | ||
| this score evaluates. | ||
| reasoning_key (Optional[str]): Key used to store the reasoning/explanation | ||
| for the score. Defaults to None. |
There was a problem hiding this comment.
Say what None means (don't include a reasoning/ CoT field)
Ditto with description (Defaults to ""The explanation for the score."")
I still think we should have a better default than "The explanation for the score." - like "Think step-by-step about what the correct score should be."
| if data.get("include_explanation") and data.get("reasoning_key"): | ||
| raise ValueError( | ||
| "Cannot include both include_explanation and reasoning_key, " | ||
| "please just use reasoning_key - include_explanation has been deprecated" # noqa: E501 |
There was a problem hiding this comment.
nit: If we're already splitting across lines let's split them within the limit rather than using noqa
| include_explanation: bool = False # Deprecated | ||
| explanation_description: Optional[str] = None # Deprecated | ||
|
|
||
| def __init__(self, **data): |
There was a problem hiding this comment.
A sad byproduct of us doing this is you lose IDE completion. Could we add the keys here?
Ditto with below.
| @pytest.mark.parametrize( | ||
| "config_class", [CategoricalScoreConfig, ContinuousScoreConfig] | ||
| ) | ||
| def test_backwards_compatibility(config_class) -> None: |
There was a problem hiding this comment.
nit: Thse could probably go in unit_tests/* ratehr than in integration tests
No description provided.