-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Don't allow customization of SchemaGenerator until interface is more stable
#10303
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
Conversation
removing from init fixing config test
Deploying pydantic-docs with
|
| Latest commit: |
0290d67
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e24b6160.pydantic-docs.pages.dev |
| Branch Preview URL: | https://no-gen-schema-customization.pydantic-docs.pages.dev |
CodSpeed Performance ReportMerging #10303 will not alter performanceComparing Summary
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||
SchemaGenerator until interface is more stableSchemaGenerator until interface is more stable
| @property | ||
| def _current_generate_schema(self) -> GenerateSchema: | ||
| cls = self._config_wrapper.schema_generator or GenerateSchema | ||
| return cls.__from_parent( | ||
| self._config_wrapper_stack, | ||
| self._types_namespace_stack, | ||
| self.model_type_stack, | ||
| self._typevars_map, | ||
| self.defs, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm wondering, what was the purpose of creating new instances, if every attribute was being passed in?
Apart from field_name_stack where a new instance is created. By the way, doesn't that break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of creating new instances was to give the model a change to inject its custom core schema generator (some subclass of GenerateSchema).
Re field_name_stack - I believe it continues to operate as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this can lead to inconsistent state, where in the process of generating a core schema for a field (and thus having one field name in the stack), we end up generating something else. However, the field names are only used when generating schema for *Validator classes, that can only be used when defining a field (meaning a new field name will be added to the stack, so it doesn't matter if the stack was reset or not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Interesting. Not sure I follow entirely - happy to take a closer look together and potentially refactor that structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance:
class Sub(BaseModel):
f: 'Forward'
Forward = int
class Model(BaseModel):
sub: SubWhen Sub is defined, schema generation fails because Forward isn't defined. when Model.sub is being defined, the field name stack is ['sub']. We try to get the core schema of Sub but it needs to be rebuilt. Previous to your change, we would create a new GenerateSchema instance with an empty field name stack. But now, we build the core schema of Sub with the same instance. When we try to build the core schema of Sub.f, the field name stack is ['sub', 'f'], so it is still valid. It can be inconsistent if we try to build something not related to a field in Sub (e.g. evaluating the type annotation of __pydantic_extra__, etc), but anyway the field name stack is only used by *Validator classes and such classes could only be used (in our example) in the context of field f, so the field name stack would be ['sub', 'f'] so all good.
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
…ntic/pydantic into no-gen-schema-customization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
| @property | ||
| def _current_generate_schema(self) -> GenerateSchema: | ||
| cls = self._config_wrapper.schema_generator or GenerateSchema | ||
| return cls.__from_parent( | ||
| self._config_wrapper_stack, | ||
| self._types_namespace_stack, | ||
| self.model_type_stack, | ||
| self._typevars_map, | ||
| self.defs, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this can lead to inconsistent state, where in the process of generating a core schema for a field (and thus having one field name in the stack), we end up generating something else. However, the field names are only used when generating schema for *Validator classes, that can only be used when defining a field (meaning a new field name will be added to the stack, so it doesn't matter if the stack was reset or not).
|
hey, just thought i would drop in with my usecase of
|
Marked as experimental in the docs: https://docs.pydantic.dev/latest/api/config/#pydantic.config.ConfigDict.schema_generator
There are lots of things that can simply be done by overriding
__get_pydantic_core_schema__on a model class or using a custom type with a custom core schema.There are a few things that were advantageous about this approach, like being able to customize how unknown type schemas were handled. That being said, I think we should: