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 2 commits 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 pdm 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?

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