Skip to content

Don't allow customization of SchemaGenerator until interface is more stable - #10303

Merged
sydney-runkle merged 13 commits into
mainfrom
no-gen-schema-customization
Sep 5, 2024
Merged

sydney-runkle merged 13 commits into
mainfrom
no-gen-schema-customization

Conversation

@sydney-runkle

Copy link
Copy Markdown
Contributor

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:

  1. In the short term, add explicit support for these use cases
  2. In the long term, solidify the API for core schema generation such that we can support and document stable ways to customize schema at the model level.

removing from init

fixing config test
@github-actions github-actions Bot added the relnotes-fix Used for bugfixes. label Sep 4, 2024
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 4, 2024 •

Copy link
Copy Markdown

Deploying pydantic-docs with Β Cloudflare Pages Β Cloudflare Pages

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

View logs

@codspeed

codspeed Bot commented Sep 4, 2024 •

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #10303 will not alter performance

Comparing no-gen-schema-customization (0290d67) with main (cb8d300)

Summary

βœ… 50 untouched benchmarks

@github-actions

github-actions Bot commented Sep 4, 2024 •

Copy link
Copy Markdown
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
Β Β pydantic
Β Β __init__.py 399
Β Β config.py
Β Β type_adapter.py
Β Β pydantic/_internal
Β Β _generate_schema.py
Project Total Β 

This report was generated by python-coverage-comment-action

@sydney-runkle sydney-runkle added relnotes-change Used for changes to existing functionality which don't have a better categorization. relnotes-performance Used for performance improvements. and removed relnotes-fix Used for bugfixes. labels Sep 4, 2024
@sydney-runkle sydney-runkle changed the title WIP: Don't allow customization of SchemaGenerator until interface is more stable Don't allow customization of SchemaGenerator until interface is more stable Sep 4, 2024
@sydney-runkle
sydney-runkle marked this pull request as ready for review September 4, 2024 17:30
Comment thread pydantic/_internal/_generate_schema.py Resolved
Comment thread pydantic/_internal/_generate_schema.py Resolved
Comment on lines -409 to -418
@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,
)

Copy link
Copy Markdown
Member

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?

@sydney-runkle sydney-runkle Sep 5, 2024 •

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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).

Copy link
Copy Markdown
Contributor Author

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.

@Viicos Viicos Sep 5, 2024 •

Copy link
Copy Markdown
Member

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: Sub

When 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.

@adriangb adriangb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

Comment on lines -409 to -418
@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,
)

Copy link
Copy Markdown
Member

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).

@sydney-runkle
sydney-runkle merged commit 6db2a0c into main Sep 5, 2024
@sydney-runkle
sydney-runkle deleted the no-gen-schema-customization branch September 5, 2024 19:49
@KotlinIsland

KotlinIsland commented Mar 17, 2025 •

Copy link
Copy Markdown
Contributor

hey, just thought i would drop in with my usecase of schema_generator/GenerateSchema, i need to deal with an api where there are many uncomfortable features that affect the entire api surface:

  • boolean values are represented with an "X" for True, and are not present at all for False
  • time/date/datetimes all use consistent, yet non-standard formatting, and return "--" instead of null

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

Labels

relnotes-change Used for changes to existing functionality which don't have a better categorization. relnotes-performance Used for performance improvements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants