-
Notifications
You must be signed in to change notification settings - Fork 1.9k
deprecate: Dataset without name parameter #4862
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
Changes from 4 commits
4a7c58d
13c41b2
97c839a
03856a0
cb4684a
944d9df
c63ebda
7c59c60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from __future__ import annotations | ||
|
|
||
|
|
||
| class PydanticEvalsDeprecationWarning(UserWarning): | ||
| """Warning emitted when a deprecated Pydantic Evals API is used. | ||
| Inherits from `UserWarning` instead of `DeprecationWarning` so that | ||
| deprecations are visible by default at runtime, following the approach | ||
| described in https://sethmlarson.dev/deprecations-via-warnings-dont-work-for-python-libraries. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, should like we should be doing something like this for Pydantic AI deprecation warnings as well. |
||
| """ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| from pydantic_evals._utils import get_event_loop | ||
|
|
||
| from ._utils import get_unwrapped_function_name, logfire_span, task_group_gather | ||
| from ._warnings import PydanticEvalsDeprecationWarning | ||
| from .evaluators import EvaluationResult, Evaluator | ||
| from .evaluators._base import BaseEvaluator | ||
| from .evaluators._run_evaluator import run_evaluator | ||
|
|
@@ -196,6 +197,7 @@ def evaluate(self, ctx: EvaluatorContext) -> bool: | |
| return ctx.output == ctx.expected_output | ||
|
|
||
| dataset = Dataset( | ||
| name='uppercase_tests', | ||
| cases=[ | ||
| Case(name='test1', inputs={'text': 'Hello'}, expected_output='HELLO'), | ||
| Case(name='test2', inputs={'text': 'World'}, expected_output='WORLD'), | ||
|
|
@@ -226,7 +228,7 @@ async def main(): | |
| """ | ||
|
|
||
| name: str | None = None | ||
| """Optional name of the dataset.""" | ||
| """Name of the dataset. Required in future versions.""" | ||
| cases: list[Case[InputsT, OutputT, MetadataT]] | ||
| """List of test cases in the dataset.""" | ||
| evaluators: list[Evaluator[InputsT, OutputT, MetadataT]] = [] | ||
|
|
@@ -245,11 +247,18 @@ def __init__( | |
| """Initialize a new dataset with test cases and optional evaluators. | ||
|
|
||
| Args: | ||
| name: Optional name for the dataset. | ||
| name: Name for the dataset. Omitting this is deprecated and will raise an error in a future version. | ||
| cases: Sequence of test cases to include in the dataset. | ||
| evaluators: Optional sequence of evaluators to apply to all cases in the dataset. | ||
| report_evaluators: Optional sequence of report evaluators that run on the full evaluation report. | ||
| """ | ||
| if name is None: | ||
| warnings.warn( | ||
| 'Omitting the `name` parameter is deprecated. Please provide a name for your `Dataset`.', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The deprecation message says "will raise an error in a future version" — since the version policy states deprecated features are removed in V2, it would be more helpful to users to say something like Also, |
||
| PydanticEvalsDeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
Comment on lines
+255
to
+260
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Comment on lines
+255
to
+260
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Incomplete deprecation migration across docs The PR updates Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| case_names = set[str]() | ||
| for case in cases: | ||
| if case.name is None: | ||
|
|
@@ -727,9 +736,9 @@ def _from_dataset_model( | |
| cases.append(row) | ||
| if errors: | ||
| raise ExceptionGroup(f'{len(errors)} error(s) loading evaluators from registry', errors[:3]) | ||
| result = cls(name=dataset_model.name, cases=cases, report_evaluators=report_evaluators) | ||
| if result.name is None: | ||
| result.name = default_name | ||
| # Use default_name if no name was provided in the serialized data | ||
| name = dataset_model.name if dataset_model.name is not None else default_name | ||
| result = cls(name=name, cases=cases, report_evaluators=report_evaluators) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is correct — it avoids the deprecation warning when loading from file (where
Comment on lines
+739
to
+741
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Deprecation warning When Was this helpful? React with 👍 or 👎 to provide feedback.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude: |
||
| result.evaluators = dataset_evaluators | ||
| return result | ||
|
|
||
|
|
||
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 choice following the
UserWarningpattern for library deprecations. Pydantic core usesPydanticDeprecatedSince20(inheritingDeprecationWarning) — was there a deliberate decision to diverge from that pattern forpydantic_evals, or would it be worth aligning? Either way is reasonable, but if the intent is to eventually align with Pydantic core's approach, it's worth noting. @DouweM any preference here?