pydantic icon indicating copy to clipboard operation
pydantic copied to clipboard

Add support for deprecated fields

Open Viicos opened this issue 2 years ago • 24 comments

Change Summary

Starting implementing https://github.com/pydantic/pydantic/issues/2255

The new deprecated decorator could now be used as an annotation to mark fields as deprecated. The CPython implementation got tweaked to allow introspection by defining the decorator as a class instead as a function.

This PR is of course subject to change depending on how you want this to be implemented in Pydantic.

The tests basically show how it is possible to mark a field as being deprecated. A couple questions:

  • Should we allow deprecated to be used as an argument to Field/computed_field (as it is currently in this PR)? Or should we only allow Annotated[..., deprecated(...)]/@deprecated(...) on a property?
  • If yes, should we allow msg to be used here as well? (instead of bool currently).
  • Should a runtime deprecation warning be raised when accessing a deprecated field? (This is already handled when used on a computed_field)

Additional context: https://github.com/annotated-types/annotated-types/pull/34#issuecomment-1826859535

Related issue number

Checklist

  • [ ] The pull request title is a good summary of the changes - it will be used in the changelog
  • [ ] Unit tests for the changes exist
  • [ ] Tests pass on CI
  • [ ] Documentation reflects the changes where applicable
  • [ ] My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

Viicos avatar Nov 27 '23 20:11 Viicos

CodSpeed Performance Report

Merging #8237 will not alter performance

Comparing Viicos:deprecated (4acb83a) with main (bbf0724)

Summary

✅ 10 untouched benchmarks

codspeed-hq[bot] avatar Dec 02 '23 15:12 codspeed-hq[bot]

Please review (still in draft because I'm waiting for a stable release of typing extensions)

Viicos avatar Dec 02 '23 16:12 Viicos

@Viicos,

Changes are looking good overall thus far. My initial thoughts:

Should we allow deprecated to be used as an argument to Field/computed_field (as it is currently in this PR)? Or should we only allow Annotated[..., deprecated(...)]/@deprecated(...) on a property?

Yes, we want to include deprecated as a parameter of Field

If yes, should we allow msg to be used here as well? (instead of bool currently).

The fewer new parameters we add to Field, the better, so I'm in favor of skipping the msg parameter in addition to the deprecated param. I would support, however, defining our own Deprecated dataclass similar to others in the types.py file. Then, we could do something like:

# either
some_field = Field(deprecated=Deprecated(True, msg='blah blah')
# or
some_field = Field(deprecated=True)

Reminds me a bit of the support we have for discriminator='some str' or discriminator=Discriminator(...) for Field.

Should a runtime deprecation warning be raised when accessing a deprecated field? (This is already handled when used on a computed_field)

I think yes, but let's see what @samuelcolvin thinks.

Additionally, we'll need to update both the API docs and the concepts docs with information about the various ways in which to use deprecated.

Depending on when 4.9.0 comes out for typing-extensions and our release timeline for 2.6.0, there's a possibility that we could include this as one of the big new feature releases in 2.6.0 🚀.

sydney-runkle avatar Dec 08 '23 16:12 sydney-runkle

Isn't having Field(deprecated=Deprecated(True, msg="blabla")) too repetitive? I don't see a valid use case of having it set to False. My proposal here would be: type deprecated as bool | str:

  • if False, no runtime warning is emitted, but "deprecated": false is set explicitly on the JSON Schema.
  • if True, it is set to true in the JSON Schema, and we either: emit no runtime warning or emit one with a default template message {field} is deprecated.
  • if str, it is set to true in the JSON Schema, and we emit a runtime warning with the provided message.

If having str | bool isn't really suitable, we could still go with our own Deprecated dataclass but with msg as the sole argument?

Additionally, we'll need to update both the API docs and the concepts docs with information about the various ways in which to use deprecated.

I'll probably add them once we get the implementation sorted out.


Would be nice to also support this on models:

from pydantic import BaseModel

from typing_extensions import deprecated

@deprecated("msg")  # Either with the decorator or model_config
class Model(BaseModel):
    pass

    model_config = {'deprecated': True}

Model.model_json_schema()
#> {'properties': {}, 'title': 'Model', 'type': 'object', 'deprecated': True}

Probably better in another PR, but would be nice to have both features landing in the same version.

Viicos avatar Dec 08 '23 16:12 Viicos

@Viicos,

That's fair, it's certainly redundant. some_field: X | Y = Field(discriminator=Discriminator(discriminator=some_func) feels a bit redundant too, but I think the case is worse for the Deprecated usage, when as you mentioned, we expect deprecated=True in most use cases.

I'm more in favor of a Deprecated dataclass, with just the msg arg as you've described. Though I see your point with the str | bool approach, I'd rather we separate the bool that's specifying the deprecated nature vs the msg used in the case of deprecated=True.

Let's wait and see what @dmontagu thinks about this API. Don't want to have you do more work than necessary.

Sounds good re docs 👍.

I agree, that'd be a nice feature on models. Indeed, a separate PR sounds like a good plan.

sydney-runkle avatar Dec 08 '23 17:12 sydney-runkle

@Viicos if we are going to do this, I'd be inclined to try to match the API for warnings.deprecated proposed in PEP 702 as closely as possible. In particular, I would lean toward deprecated: str | None = _Unset as the annotation for this keyword argument, where None(or _Unset) means not deprecated, and if a string is passed, it should serve the same purpose, i.e., provide some deprecation message.

Is that reasonable to you?

I know the decorator from that PEP has some keyword arguments that affect the emitted warnings, but I think trying to support those (not to mention emitting runtime warnings when accessing the field) is outside the scope of this feature request/implementation right now. (If we want to consider doing that, I think it would merit a bigger design discussion.)

That said, I'll just note that I think we'll regret adding support for this if dataclasses adds support for marking a field as deprecated in some future PEP, and the approach used there is not API-compatible with this.

dmontagu avatar Dec 11 '23 21:12 dmontagu

None(or _Unset) means not deprecated, and if a string is passed, it should serve the same purpose, i.e., provide some deprecation message.

Sounds good.

trying to support those [...] is outside the scope of this feature request/implementation right now. [...] I think it would merit a bigger design discussion.

Yes, I think this could be implemented later. I think for now I'll just implement emitting DeprecationWarning with a fixed stacklevel value (probably 2?). In the future, it would be nice to respect the arguments passed to deprecated, i.e.:

field: Annotated[int, warnings.deprecated("msg", category=MyCustomDepWarning, stacklevel=3)]

will emit a MyCustomDepWarning with stacklevel=3 when field is accessed. But this can be discussed later when actually implementing this functionality.

I'll just note that I think we'll regret adding support for this if dataclasses adds support for marking a field as deprecated in some future PEP, and the approach used there is not API-compatible with this.

While I agree on this, no one has started a PEP related to this feature (or the hypothetical Deprecated type hint as discussed here and on following comments, although I don't think this solution will ever happen considering deprecated was moved to warnings to avoid confusion). So it might never happen after all, and it would be unfortunate to not have this deprecated feature in Pydantic just because we are waiting on a built in feature that might never happen.

Having it implemented in Pydantic could also be an inspiration to implement it in dataclasses (e.g. in the same way it is currently done here: via Field). The dataclass_transform spec could then also be aligned to support the deprecated argument in library field specifiers, making Pydantic compatible.

Viicos avatar Dec 12 '23 13:12 Viicos

I didn't realize the typing_extensions.deprecated/warnings.deprecated was actually a class — I'm good with supporting an instance of that class as the argument to Field, so the kwarg annotation is deprecated: str | warnings.deprecated | typing_extensions.deprecated | None (can probably do some things to clean that annotation up but keep the same semantics).

The main question for me is whether it is worth allowing instances of this class to work as a standalone annotation to mark a field as deprecated, or if we should just require an annotation of Field(deprecated=deprecated(...)). I guess it's best to support adding deprecated(...) as an annotation, it's just annoying because the field version will work without any other changes, where I guess we'll have to do something specific to support a plain deprecated annotation. (I assumed you haven't added support for that yet but I didn't look closely..)

dmontagu avatar Dec 12 '23 20:12 dmontagu

Nevermind, I see you added support for it as a plain annotation and it was basically trivial 👍

dmontagu avatar Dec 12 '23 20:12 dmontagu

Nevermind, I see you added support for it as a plain annotation and it was basically trivial 👍

Yes thanks to Zac-HD who proposed a clever workaround ;)

Viicos avatar Dec 12 '23 20:12 Viicos

Now using str for deprecated. I've mirrored the logic of the @deprecated decorator, that is it should emit a warning even if msg=''. Would it make sense to have empty strings set "deprecated": true on the JSON Schema but no warning is emitted?


~~Still need to emit the deprecation warning on attribute access. I tried defining a BaseModel.__getattribute__ I end up with recursion errors because I need to access model_fields/model_computed_fields to check if there is a deprecation message for the requested attribute. Plus __getattr__ makes use of object.__getattribute__ so this is tricky to handle. Any idea where I could raise such warnings? Wrapping deprecated model fields with a descriptor could do the trick, but there's probably a better way~~

To emit the deprecation warning, I had to set __pydantic_deprecated_messages__ at the class level. It is a mapping between deprecated field names and the corresponding deprecation message. An extra __getattribute__ is added on BaseModel which will look for a deprecation message and warn if required. Having this __pydantic_deprecated_messages__ at the class level is required to avoid infinite recursion with __getattribute__.

Viicos avatar Dec 28 '23 19:12 Viicos

Please review

Had to update to latest pyright for deprecated to be recognized as a class. I will make a separate PR to fix new errors

Viicos avatar Dec 29 '23 18:12 Viicos

@tiangolo sorry for the ping, I see there are some failures on the fastapi tests. How this feature is going to play with FastAPI? Will it be supported out of the box?

Viicos avatar Jan 12 '24 14:01 Viicos

@Viicos,

Could you please review the conflicting files in this PR when you get a chance? Thanks a bunch!

sydney-runkle avatar Jan 25 '24 17:01 sydney-runkle

I somehow totally missed/misread this comment, will apply changes soon

Viicos avatar Jan 25 '24 19:01 Viicos

Docs probably require more work, maybe a dedicated section in concepts?

Viicos avatar Jan 29 '24 09:01 Viicos

Docs probably require more work, maybe a dedicated section in concepts?

@Viicos, Yep, a concepts section would be great. Perhaps in the Concepts / Fields section?

Changes look good overall. Will do one last review once you've added docs :).

sydney-runkle avatar Jan 29 '24 14:01 sydney-runkle

Working on getting a patch release out for 2.6 with some fixes, then will review this in more detail once that's out :).

sydney-runkle avatar Jan 30 '24 13:01 sydney-runkle

I know the decorator from that PEP has some keyword arguments that affect the emitted warnings, but I think trying to support those (not to mention emitting runtime warnings when accessing the field) is outside the scope of this feature request/implementation right now. (If we want to consider doing that, I think it would merit a bigger design discussion.)

Thinking about this, I believe the implementation could be pretty straightforward. There can still be things to be discussed (i.e. is warnings.deprecated the only way to provide a category and stacklevel?), but I think I could get some time to work on it as part as the next release after this one gets merged.

If you still believe it requires more discussion, then we can delay a bit the implementation and could open an issue/discussion to see how this should be handled

Viicos avatar Feb 06 '24 17:02 Viicos

As a proof of concept (no tests added), I pushed a commit that can support stacklevel and category from deprecated. It however requires typing_extensions>=4.9.0 (a DeprecatedFieldInfo class could be added otherwise, holding the msg/category/stacklevel attributes).

Viicos avatar Feb 09 '24 18:02 Viicos

The new implementation seems to fix the performance issue, at least for non deprecated fields:

import time
import warnings

from pydantic import BaseModel, Field


class MyModel(BaseModel):
    x: int = Field(deprecated='test')
    y: int


my_model = MyModel(x=1, y=1)

t0 = time.time()
with warnings.catch_warnings():
    warnings.simplefilter('ignore')
    for _ in range(100_000):
        my_model.x
        # my_model.y
t1 = time.time()
print(t1 - t0)

With my_model.x: 0.05495929718017578 With my_model.y: 0.0045850276947021484


Test failure is probably because typing-extensions 4.10.0rc1 got released, parsing fails with release candidates. I'll fix it

Viicos avatar Feb 20 '24 19:02 Viicos

Please review

I opened https://github.com/tiangolo/fastapi/issues/11182 to track FastAPI support

Viicos avatar Feb 22 '24 13:02 Viicos

@Viicos,

Thanks! I'll do an in depth review this afternoon :).

sydney-runkle avatar Feb 23 '24 12:02 sydney-runkle

To summarize things a bit:

  • I reverted https://github.com/pydantic/pydantic/pull/8237/commits/cf06a7322d59aefc2e3ca607f0c5ee66d28f6757 (support for category and stacklevel) for now, but can be added back
  • Once this one gets merged, ideally we could have support for deprecated models, and maybe Pydantic dataclasses?
  • CI will fail with FastAPI until it is updated to handle this new feature. Once this PR is approved, I can find time to implement support in FastAPI

Viicos avatar Feb 23 '24 12:02 Viicos

@Viicos,

Could you please move the pyproject.toml and package version parsing changes to a different PR, just so that we can keep this one entirely focused on the new deprecated field work?

Happy to review those changes as well, I just think having 2 PRs will make things a bit tidier.

sydney-runkle avatar Feb 27 '24 01:02 sydney-runkle

  • Reverted the parse_mypy_version changes and reverted metadata changes (will open a new PR).
  • Replaced from_deprecated_decorator with the proposed solution
  • I added some extra tests with validators and validate_assignment.

One test will fail because a deprecation warning is emitted here:

@model_validator(mode='after')
def validate_x(self) -> Self:
    self.x = self.x * 2

I'm wondering if we should use filterwarnings when calling validators internally? (I think it makes sense, users will want the deprecation warning to be emitted only when the field is accessed)

Viicos avatar Feb 27 '24 08:02 Viicos

  • [x] Reverted the parse_mypy_version changes and reverted metadata changes (will open a new PR).

Awesome!

  • [x] Replaced from_deprecated_decorator with the proposed solution

Great! I've opened https://github.com/pydantic/pydantic/issues/8902 to address @dmontagu's request to have ComputedFieldInfo be explicitly non-public.

I added some extra tests with validators and validate_assignment.

Perfect!

I'm wondering if we should use filterwarnings when calling validators internally? (I think it makes sense, users will want the deprecation warning to be emitted only when the field is accessed)

Good question. I think it probably makes sense to put that burden on the user, but let me double check and get back to you in just a few minutes!

Given all of this, once we resolve the test question posed above ^^, I'll be ready to approve!

When we release 2.7 in a few weeks, we'll certainly highlight your spectacular work on this!! 🚀 Make sure to keep an eye out for that!

sydney-runkle avatar Feb 27 '24 13:02 sydney-runkle

@Viicos,

Ah, and just one quick follow up question - were there changes to the parse_mypy_version function that you wanted to include in a different PR?

sydney-runkle avatar Feb 27 '24 13:02 sydney-runkle

I'm wondering if we should use filterwarnings when calling validators internally? (I think it makes sense, users will want the deprecation warning to be emitted only when the field is accessed)

Good question. I think it probably makes sense to put that burden on the user, but let me double check and get back to you in just a few minutes!

Ah ok, yes let's leave this to the user. However, could you please add a section to the docs explaining how a user could filter these warnings if they desire to use a deprecated field in a model validator? Thanks a bunch!

sydney-runkle avatar Feb 27 '24 14:02 sydney-runkle

Ah, and just one quick follow up question - were there changes to the parse_mypy_version function that you wanted to include in a different PR?

We initially wanted to make the function generic to parse other package versions, but turns out it doesn't work properly in some cases (with release candidates, but probably with a lot of features from PEP 440). That's why I reverted it to be specific for mypy, and if parsing package versions (and comparing them) is needed in tests, packaging should be used (as I did in this PR) as it will handle every edge case for us.


Ah ok, yes let's leave this to the user. However, could you please add a section to the docs explaining how a user could filter these warnings if they desire to use a deprecated field in a model validator? Thanks a bunch!

My reasoning for filtering warnings when calling validators was:

  • You define your model with a property a, that needs to be manually validated by assigning a new value:
class Model(BaseModel):
    a: int = 2

    @model_validator(mode="after")
    def validate_model(self) -> Self:
        self.a = self.a * 2
  • Later on, you deprecated the a field in favor of a_new, with similar validation:
class Model(BaseModel):
    a: Annotated[int, deprecated("use 'a_new'")] = 2
    a_new: int = 5

    @model_validator(mode="after")
    def validate_model(self) -> Self:
        self.a = self.a * 2
        self.a_new = self.a_new * 2.5
  • When instantiating Model, I wouldn't want a deprecation warning to be emitted even if haven't provided any value for a, e.g. Model(a_new=3) [^1], especially as an "external" user of this Model who has no knowledge about the internal validation being performed. What do you think? (Of course, accessing Model(a_new=3).a should still emit the warning).

[^1]: This makes me think: should a deprecation warning be raised when doing Model(a=...)?

Viicos avatar Feb 27 '24 16:02 Viicos