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

protected_namespaces isn't as useful as could be #9177

Open
3 of 13 tasks
antoine-lizee opened this issue Apr 5, 2024 · 5 comments
Open
3 of 13 tasks

protected_namespaces isn't as useful as could be #9177

antoine-lizee opened this issue Apr 5, 2024 · 5 comments

Comments

@antoine-lizee
Copy link

antoine-lizee commented Apr 5, 2024

Initial Checks

  • I have searched Google & GitHub for similar requests and couldn't find anything
  • I have read and followed the docs and still think this feature is missing

Description

Currently, we get 2 behaviours:

  • A field that starts with model_ will issue a warning. That's desirable in some cases but annoying in others, especially in the age of AI.
  • A field that collides with a pydantic field will issue an error. That's desirable in most cases.

Currently, setting protected_namespaces=() in the config removes both. (Note: it is not clear in the documentation that the error is silenced too)

class Example(BaseModel):
     model_config = ConfigDict(protected_namespaces=())
     model_dump: str

#  -> UserWarning: Field name "model_dump" shadows an attribute in parent "BaseModel"; 

This is a problem because silencing the warning in the cases that are ok is now very dangerous (you also silence a useful error).

Potential suggestions:

  • 1/ Keep the error if collision when setting protected_namespaces=(). It seems that it was the initial behaviour some time ago, so my guess is that the changes have been made for a reason (probably the rare of cases when you want to override)
  • 2/ Remove the warnings for protected namespaces at baseline, and only keep the error behaviour. (-> the developer only removes namespace protection when they need to overwrite a protected field)
  • 3/ Give more fine-grained control: have two configs, one for the warnings related to the namespace and one for the error in case of collision
  • 4/ [also] allow to change the prefix: starting with pydantic_ will likely remove 99% of the issue. (see Prefix all protected namespaces with "pydantic_" #7031 too)

It seems that 3/ is the most backward compatible option, but it adds one more config option...

What do you guys think?

( Side note: I'm also not sure of the point of protected_namespaces in general. What are good use cases for setting other protected namespaces? It seems that I can only reproduce the warning behaviour on other namespaces, which I also get when defining the model unless I set the config after the fields somehow )

Affected Components

@sydney-runkle
Copy link
Member

@antoine-lizee,

Thanks for the well thought out feature request. I'm in favor of 3, bc as you mentioned, it's the most backwards compatible option. That being said, let's see what the team thinks before we move ahead with a PR. Will get back to you shorlty!

@yermandy
Copy link

I agree with @antoine-lizee that it's quite annoying when using it in a deep learning project. I think pydantic_ prefix would be a much better solution

@sydney-runkle
Copy link
Member

@yermandy @antoine-lizee,

Changing everything to pydantic_ would be quite the breaking change. Let's go with option 3 for now, which adds more support, but isn't breaking in nature.

@antoine-lizee
Copy link
Author

Yeah it makes sense. I think 3/ is the best short-term.

Coming back to the pydantic_ suggestion, my proposal was to allow for it with a config change. Not sure it would be possible, but that could be a good direction until an official deprecation later.

Something like:

class BaseModelForThisRepo(BaseModel):
    model_config = ConfigDict(model_prefix_alias=`pydantic_`, remove_collision_errors_for_namespace=`model_`)

@astoff
Copy link

astoff commented Apr 29, 2024

As an alternative (or additionally), why not make the model_... methods plain functions in the pydantic namespace? Clearly every non-_ prefix for method names one picks will eventually cause a clash for someone somewhere.

One extra advantage is that this would allow calling pydantic.model_dump(obj) not only for Pydantic models, but also when obj is an Optional[Model], or is a list of models, or any other structure containing models.

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

No branches or pull requests

4 participants