Skip to content
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

Raise warnings when deprecated fields are filled at model instantiation #1551

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

changhc
Copy link
Contributor

@changhc changhc commented Nov 21, 2024

Change Summary

Raise warnings when deprecated fields receive values at model instantiation. My previous attempt at pydantic/pydantic#10865 was rejected because it led to performance issues, so I'm hoping that by moving the logic to pydantic-core will work better. At least doing it here means that the code won't need to loop over the input values one more time.

One extra config is added to the model-field schema: deprecation_msg (Option<String>). A field is marked as deprecated when this config value is present. The actual deprecation message will be passed in from pydantic; this keeps handling the type of the deprecation message simple, and most importantly, there is already logic over there dealing with possible cases of deprecation messages and thus we should not duplicate the logic here.

Also tested with pydantic and it worked.

Related issue number

Part of pydantic/pydantic#8922

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (ab503cb) to head (a19365c).
Report is 240 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1551      +/-   ##
==========================================
- Coverage   90.21%   89.62%   -0.59%     
==========================================
  Files         106      112       +6     
  Lines       16339    17892    +1553     
  Branches       36       40       +4     
==========================================
+ Hits        14740    16036    +1296     
- Misses       1592     1836     +244     
- Partials        7       20      +13     
Files with missing lines Coverage Δ
python/pydantic_core/core_schema.py 94.84% <100.00%> (+0.07%) ⬆️
src/validators/model_fields.rs 95.18% <100.00%> (-1.29%) ⬇️

... and 54 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6472887...a19365c. Read the comment docs.

Copy link

codspeed-hq bot commented Nov 21, 2024

CodSpeed Performance Report

Merging #1551 will not alter performance

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

Summary

✅ 155 untouched benchmarks

@changhc changhc marked this pull request as ready for review November 21, 2024 21:29
@changhc
Copy link
Contributor Author

changhc commented Nov 21, 2024

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.

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me, we can get access to the warning type via the PyO3 API and let that handle the most efficient way to do it 😄

@@ -184,6 +186,10 @@ impl Validator for ModelFieldsValidator {
// extra logic either way
used_keys.insert(lookup_path.first_key());
}
if let Some(msg) = &field.deprecation_msg {
let deprecation_warning_type = py.import_bound("builtins")?.getattr("DeprecationWarning")?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try this instead:

Suggested change
let deprecation_warning_type = py.import_bound("builtins")?.getattr("DeprecationWarning")?;
let deprecation_warning_type = pyo3::exceptions::PyDeprecationWarning::type_object_bound(py);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. This works. Thanks! There's another UserWarning imported in the same way somewhere in the code base. I can create another PR to change that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a great follow-up, yes please 👍

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Viicos if you're happy with the core schema & test then let's merge this, the Rust code LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants