Skip to content

Move core schema generation logic for path types inside the GenerateSchema class #10846

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

Merged
merged 11 commits into from
Jan 2, 2025

Conversation

sydney-runkle
Copy link
Contributor

@sydney-runkle sydney-runkle commented Nov 14, 2024

Some (verbose) context: #10036

I'm trying to remove the use of prepare_pydantic_annotations_for_known_type internally. We call this function in order to to pull metadata off of types and inject said metadata's constraints into core schema. This is bad practice, as we generally advertise being religious about respecting metadata order when building schema, but we violate that principle here.

Though these checks don't drag much on performance, they do significantly clutter our internal schema generation logic. They represent the old pattern (we have since deprecated) of supporting a __pydantic_prepare_annotations__ method.

For example, we used to pull string constraint data like min_length off of a Path like annotation + apply that to the inner core_schema.str_schema() or core_schema.bytes_schema().

This removes support for constraints like max_length, min_length, and strip_whitespace or other string constraints on Path types. Users should implement these constraints with custom validators. This should be advertised in the v2.11 changelog.

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Nov 14, 2024
Copy link

cloudflare-workers-and-pages bot commented Nov 14, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: b2b8f7e
Status: ✅  Deploy successful!
Preview URL: https://b439c509.pydantic-docs.pages.dev
Branch Preview URL: https://perf-remove-prep-anns.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #10846 will not alter performance

Comparing perf-remove-prep-anns (b2b8f7e) with main (9553fa4)

Summary

✅ 46 untouched benchmarks

@sydney-runkle sydney-runkle changed the title WIP: removing prepare pydantic annotations logic Removing prepare pydantic annotations (for known types) logic Nov 14, 2024
Copy link
Contributor

github-actions bot commented Nov 14, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic/_internal
  _generate_schema.py 485, 498-501, 503, 506-507
  _std_types_schema.py
Project Total  

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

Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

What does this tell us about our annotation application in general?

I think it reveals a limitation of our current annotation application. Because paths could be considered as a custom type (i.e. there's no core schema that represents this type, we currently build the validation logic with a custom after validator), we face the limitation of not having access to the annotated metadata as described here.

The current min/max_length_validator function is not going to scale well in the future if we ever want to add support for min/max_length for another type similar to Path (and that type similarly doesn't have a core schema type for it).

I'll note that while this limitation shows up here with a type without a proper core schema for it, we also face weird behaviors with the following (and unlike Path, int has a core schema type):

def val(v):
    print('val called')
    return v

class Model(BaseModel):
    a: Annotated[int, AfterValidator(val), Field(gt=0)]

Model(a=-1)
#> val called
#> ValidationError raised
  • the after validator is called before the gt validation, which imo is not what should happen.
  • as a consequence, the gt constraint isn't represented in the int_schema(), but instead the greater_than_validator Python fallback validator function is used.

On another note, maybe we should discuss how we validate "concrete" vs "abstract" types. For instance, for paths, we use pathlib.PurePath for os.PathLike annotations, but this is an opinionated choice and users could expect Path to be used instead.

This relates to the behavior of Iterable: we currently validate these by creating a ValidationIterator instance, and people complained about it. Maybe we should have a way of customizing which "concrete" implementation should be used for "abstract" types?

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
@sydney-runkle sydney-runkle added relnotes-change Used for changes to existing functionality which don't have a better categorization. and removed relnotes-fix Used for bugfixes. labels Nov 21, 2024
@sydney-runkle sydney-runkle changed the title Removing prepare pydantic annotations (for known types) logic Removing prepare pydantic annotations (for known types) logic - Path types Nov 21, 2024
@sydney-runkle sydney-runkle requested review from Viicos and removed request for adriangb January 2, 2025 15:53
@sydney-runkle sydney-runkle added the third-party-tests Add this label on a PR to trigger 3rd party tests label Jan 2, 2025
@Viicos Viicos changed the title Removing prepare pydantic annotations (for known types) logic - Path types Move core schema generation logic for path types inside the GenerateSchema class Jan 2, 2025
@sydney-runkle sydney-runkle merged commit aa85c3a into main Jan 2, 2025
89 checks passed
@sydney-runkle sydney-runkle deleted the perf-remove-prep-anns branch January 2, 2025 17:25
recursiveAF pushed a commit to recursiveAF/pydantic that referenced this pull request Feb 22, 2025
…Schema` class (pydantic#10846)

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
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. third-party-tests Add this label on a PR to trigger 3rd party tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants