Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

CodSpeed Performance Report

Merging #1551 will not alter performance

_{Comparing changhc:8922-init-deprecated-field (a19365c) with main (5c96bec)}

Summary

✅ 155 untouched benchmarks

Please review.

Meanwhile, I'd like to ask the reviewer's opinion on how we can make this thing "less slow". Since this implementation calls import, it will surely affect performance. Codspeed pointed out performance regression with my first commit because there I called import once and for all fields even when there were no deprecated fields. Now that import is moved so that it is called only when a warning needs to be raised. Performance looks fine according to the report, but this is because we don't have any benchmark for deprecated fields.

Since we know that performance will definitely be impacted, we can try to mitigate this. For instance, instead of calling import each time a deprecated field is filled, we can collect warnings and raise them all together in one warning at the end. There are some issues with this approach though. First, the final warning message will need to be more specific because all deprecation messages will be mixed up. We will need to indicate which fields are deprecated and which message belongs to which field for clarity, but this will be inconsistent with what is implemented in pydantic now, i.e. print the deprecation warning only without any additional information. Maybe we can just implement that here and change pydantic later on.

Another issue is when that final warning should be raised. Currently ModelFieldsValidator operates in a "fail early" fashion, i.e. when any field value fails validation, the whole model validation is terminated. Even if we raise the deprecation warning right before any error return, we won't be able to mention all deprecated fields unless we iterate through all fields separately first, but this will probably lead to performance issues. If we don't collect all deprecation warning but simply raise whatever we have observed, the user experience might not be good because users will be like "finding out the remaining "undetected" deprecation fields after fixing validation issues" instead of being able to see all issues upfront and fixing them in one go. This issue already exists with the current implementation though.

So, some offline discussion with @Viicos:

• We were a bit unsure if we can merge this without first deciding on the Python API we'll add to the pydantic package. Do we want to unconditionally add warnings on instantiation? This would sort of be a breaking change for users of deprecated fields. Maybe we need a more sophisticated opt-in; perhaps discussion on More support for deprecated fields/models pydantic#8922 is needed
• Does this need to support dataclasses and typed dicts too?

For the first point: I'll gather comments in that issue.

For the second point: I think we should because deprecated fields should behave in the same way everywhere. I took a quick look at the relevant validators and I think the implementation will be quite similar. I think we can cover them in separate PRs. That would make things easier in case we need to revert anything.