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

Update deprecated pydantic model Config class to model_config attribute #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spwoodcock
Copy link

@spwoodcock spwoodcock commented Nov 21, 2024

This PR

  • Pydantic v2 deprecated the Config class on BaseModel:
PydanticDeprecatedSince20: Support for class-based `config` is deprecated, use ConfigDict instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.1.1/migration/
    warnings.warn(DEPRECATION_MESSAGE, DeprecationWarning)

Not to mention the static type checking complaints about overriding Config:

"Config" overrides symbol of same name in class "Model"
  "pyodk._endpoints.bases.FrozenModel.Config" is not assignable to "pyodk._endpoints.bases.Model.Config"
  Type "type[pyodk._endpoints.bases.FrozenModel.Config]" is not assignable to type "type[pyodk._endpoints.bases.Model.Config]"
  • Instead the model_config attribute should be used.
  • Instead of importing ConfigDict in every file, I instead created a FrozenModel class to be used:
class Model(BaseModel):
    """Base configuration for data model classes."""

    model_config = ConfigDict(arbitrary_types_allowed=True, validate_assignment=True)


class FrozenModel(Model):
    """Make the base configuration model faux-immutable.
    
    NOTE in pydantic v2 inherited model_config are *merged*.
    """

    model_config = ConfigDict(frozen=True)

What has been done to verify that this works as intended?

image

Why is this the best possible solution? Were any other approaches considered?

  • The Config class will be removed in pydantic v3, making the upgrade of this lib more difficult.
  • Upgrading things piece by piece is good!

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No change.

Do we need any specific form for testing your changes? If so, please attach one.

Nope.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Nope.

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyodk tests and ruff check pyodk tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

@lindsay-stevens
Copy link
Contributor

@spwoodcock thanks but please open a forum ticket to discuss proposed changes before launching in to a PR - you acknowledged this process in March this year. It is complicated further by opening 2x more PRs based on this one. I would also ask that you run ruff before committing so there isn't a proliferation of separate lint commits - about half of the commits across the 3 PRs you opened today are for formatting.

Is your goal to silence this particular deprecation warning? Upgrading pydantic to be ready for v3 will require more changes - I can think of at least the use of pydantic.v1 validators. The migration guide is rather long so there's probably more, I'm hoping that their bump-pydantic tool can help. Also if pyodk is being updated to support pydantic v3 then it would make sense to widen the pyodk requirements range to allow v3.

@spwoodcock
Copy link
Author

spwoodcock commented Nov 21, 2024

@lindsay-stevens yes you are right, I saw what I wrote previously, my bad entirely 😓

I'm just so used to this being the typical workflow for any other project I contribute to and got carried away.
Someone I worked with once told me: PRs are always preferred to issues. But I guess that's not the case in every scenario!

Anyway, I was doing this work regardless & I felt in the right headspace to just get it done now. It probably wouldn't have been done had I left it a few days, as something else would probably require my attention. Please feel free to close, modify, merge, whatever! 😄

@spwoodcock
Copy link
Author

spwoodcock commented Nov 21, 2024

Regarding you other points:

I would also ask that you run ruff before committing so there isn't a proliferation of separate lint commits - about half of the commits across the 3 PRs you opened today are for formatting.

The instructions to ruff format pyodk tests do not actually fix all the lint issues, despite that being the instruction in the PR template.
It's also necessary to run ruff check --fix tests pyodk too, to fix things like import sorting etc.

But regardless, the commits in a PR are a bit irrelevant if you do a squash merge no?
Would you prefer I do a reset and push as a single commit?

Is your goal to silence this particular deprecation warning? Upgrading pydantic to be ready for v3 will require more changes - I can think of at least the use of pydantic.v1 validators. The migration guide is rather long so there's probably more, I'm hoping that their bump-pydantic tool can help. Also if pyodk is being updated to support pydantic v3 then it would make sense to widen the pyodk requirements range to allow v3.

You are completely right, a migration to Pydantic v3 would take more work. As you say, I saw the v1 validators, but didn't want to dive into that now. Pydantic v3 is not released and as far as I know not on the near horizon. Is there any downside to fixing deprecations from v1 --> v2, considering we are using v2 now?

@spwoodcock spwoodcock force-pushed the fix/pydantic-model-config branch from 0596413 to 49ad17c Compare February 11, 2025 19:49
@lindsay-stevens
Copy link
Contributor

Could you please elaborate on / explain the FrozenModel comments?

Make the base configuration model faux-immutable.

What does "faux-immutable" mean? I think something is immutable or it is not - unless you mean it's somehow pretending to be immutable? Should we not expect it to be immutable? Or is this kind of echoing the idea of "frozen" in the sense of "frozendict" which could probably be modified anyway if one was so inclined, or the class annotation of frozen=True?

NOTE in pydantic v2 inherited model_config are merged.

By "merged" do you mean they're done like old_conf.update(new_conf) (assuming both are dicts)? Do you mean that they were merged but in v3+ they wont be? What does that mean in practical terms for using the class, both now an in pydantic 3+?

@spwoodcock
Copy link
Author

I wrote this three months ago and I have forgotten now 😂

Not great comments when they don't immediately remind you why you wrote them

@spwoodcock
Copy link
Author

spwoodcock commented Feb 12, 2025

Pydantic has excellent docs thankfully 👍

https://docs.pydantic.dev/latest/concepts/models/#faux-immutability

The key is here: In Python, immutability is not enforced. Developers have the ability to modify objects that are conventionally considered "immutable" if they choose to do so. So faux immutability would mean raising an error when mutation is attempted, instead of silently allowing it. Not truly immutable, but kinda

@spwoodcock
Copy link
Author

spwoodcock commented Feb 12, 2025

As for this comment NOTE in pydantic v2 inherited model_config are merged. I would assume I meant NOTE in pydantic v2 inherited model_config are merged, while in v1 they were overwritten.

@spwoodcock
Copy link
Author

Also, sorry about this, but I'm only trying to help.

I know its text and impossible to infer intent, but for some reason your tone always seems a bit abrasive.

Hopefully I'm misinterpreting that.

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

Successfully merging this pull request may close these issues.

2 participants