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

Add CoreSchemaType Literal #389

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Feb 9, 2023

No description provided.

)
assert s.to_python(123, mode='json', exclude={1: {2}}) == (
"123 info=SerializationInfo(include=None, exclude={1: {2}}, mode='json', by_alias=True, exclude_unset=False, "
"exclude_defaults=False, exclude_none=False, round_trip=False)"
'exclude_defaults=False, exclude_none=False, round_trip=False)'
Copy link
Collaborator Author

@dmontagu dmontagu Feb 9, 2023

Choose a reason for hiding this comment

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

needed to change this to make make pass, here and elsewhere that quotes changed in test files

@dmontagu dmontagu requested a review from samuelcolvin February 9, 2023 16:49
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 9, 2023

CodSpeed Performance Report

Merging #389 add-core-schema-type-literal (2572822) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 85 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

@dmontagu
Copy link
Collaborator Author

dmontagu commented Feb 9, 2023

I know we talked about making all CoreSchema members inherit from a common type as a way to improve mypy performance. I'm happy to try that, though would have two questions:

  • Today, is there anywhere that we rely on an exhaustive check that we've handled all members of CoreSchema that doesn't involve a runtime catch of mishandled cases? With a Union, I think in principle mypy could explicitly check that all cases were handled. I won't be surprised if we don't do that anywhere today, and if it's not worth bothering with, but changing CoreSchema to be a base type (rather than a Union) would eliminate the ability to do that, and also make it harder to iterate over the list of all CoreSchema types. Not sure if you intended to handle things that way; did you imagine having the base class and union as separate things?
  • How do you suggest I check the impact on mypy performance? Is it as simple as making the change and just running mypy (or whatever make mypy does, minus the disable-recursive-aliases thing?). I guess there is no make mypy for pydantic_core; do I need to build pydantic_core, then import from that in pydantic, and run make mypy on the pydantic side (possibly with recursive aliases disable off)?

@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023

Codecov Report

Merging #389 (2572822) into main (27a3367) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #389   +/-   ##
=======================================
  Coverage   95.31%   95.31%           
=======================================
  Files          90       90           
  Lines       10510    10511    +1     
  Branches        8        8           
=======================================
+ Hits        10018    10019    +1     
  Misses        492      492           
Impacted Files Coverage Δ
pydantic_core/__init__.py 100.00% <100.00%> (ø)
pydantic_core/core_schema.py 98.78% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a3367...2572822. Read the comment docs.

@samuelcolvin
Copy link
Member

Maybe @cdce8p has some thoughts here as he created a related pr #387?

@cdce8p
Copy link
Contributor

cdce8p commented Feb 9, 2023

How do you suggest I check the impact on mypy performance?

Here is what I did for #387:

  • Create a file test.py with
from pydantic import BaseModel
  • Install pydantic 2.0.0.dev.0 and mypy==1.0.0
  • Run time mypy --no-incremental test.py
  • If it takes <1min it should be fixed. During my testing the current version was always >1:30min when I aborted the run, whereas with Improve mypy typing performance #387 it finished in 15-30s.
  • Make changes to pydantic-core.
  • Reinstall it. Necessary as it's compiled!
  • Run mypy again -- repeat

I know we talked about making all CoreSchema members inherit from a common type as a way to improve mypy performance.

I did some initial testing in #387. The issue I encountered was that TypedDict doesn't allow for extra keys. Thus to make it typecheck, you probably need to create a separate TypedDict with all possible keys and total=False but that's obviously not as type safe as the Union. I know too little about the internals of pydantic / pydantic-core to judge the impact of it which is why I closed the PR.

If you just want to check if the typing would work, you could also try running make pyright. That should give you feedback fairly quickly and doesn't require reinstalls between changes.

Anyway, feel free to reuse any of the work I did #387 if that's of any help to you.

@dmontagu
Copy link
Collaborator Author

dmontagu commented Feb 9, 2023

@cdce8p thank you very much for sharing, this looks like it will be helpful, at least if I end up reaching the same conclusions as you I won't have to spend as much time as I would otherwise!

@dmontagu
Copy link
Collaborator Author

dmontagu commented Feb 9, 2023

(@samuelcolvin) So, @cdce8p, I checked out your branch for PR#387, and I got the following results following your testing description after pip-installing mypy:

❯ mypy --version
mypy 1.0.0 (compiled: yes)

❯ mypy pydantic_core  # this is near-instant
Success: no issues found in 4 source files

❯ time mypy --no-incremental test.py
Success: no issues found in 1 source file
mypy --no-incremental test.py  7.24s user 0.12s system 95% cpu 7.739 total

This all seems to be working properly and very performantly using mypy. However, I do see the issue with PyRight not liking it:

    ... (reportGeneralTypeIssues)
  /Users/davidmontague/repos/pydantic-core/tests/test_typing.py:126:26 - error: Expression of type "dict[str, str | dict[str, str] | ((bar: str) -> None)]" cannot be assigned to declared type "CoreSchema"
    Type "dict[str, str | dict[str, str] | ((bar: str) -> None)]" cannot be assigned to type "CoreSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "AnySchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "NoneSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "BoolSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "IntSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "FloatSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "StringSchema"
      "dict[str, str | dict[str, str] | ((bar: str) -> None)]" is incompatible with "BytesSchema"
    ... (reportGeneralTypeIssues)

For what it's worth, I found this stack overflow answer a bit enlightening on the issues: https://stackoverflow.com/a/71814659.

It would be nice if pyright would have the same behavior as mypy here, but I'm assuming they don't on purpose; have we tried bringing this up with them at all?

I had the idea that maybe we could introduce a couple aliases and try to trick the type systems so that mypy would see the aliases following #387, but pyright would see them the way it is currently on the main branch. However, I couldn't find a way to do type-checker-specific ignores (like if TYPE_CHECKING: but specific to mypy or specific to pyright). I feel that would be enough to make it work, with a bit of fiddling (not sure it would be worth the maintenance burden, but maybe if it was simple enough..). However, if there is no way to achieve this, maybe it's a moot point.

@cdce8p
Copy link
Contributor

cdce8p commented Feb 9, 2023

Could you try running mypy over the pydantic-core test files, in particular pydantic-core/tests/test_typing.py?
With the changes from #387

@dmontagu
Copy link
Collaborator Author

dmontagu commented Feb 9, 2023

@cdce8p Yep, that failed. (I had to get rid of the multiple : CoreSchema annotations to get it to check properly, but it gave the expected fails.)

I wouldn't be surprised if it was possible to do something with a mypy plugin here, but I'm not sure that's a good idea even if it is possible (big maintenance burden).

@samuelcolvin I'm inclined to drop any attempt to address the mypy performance stuff as part of this PR, and just leave that open for discussion separately.

@dmontagu
Copy link
Collaborator Author

dmontagu commented Feb 9, 2023

I would still like to merge this PR adding the CoreSchemaType literal though if no objections from either of you. Happy to refactor in the future as appropriate if we find a better path for performance that somehow influences this.

@samuelcolvin
Copy link
Member

Agreed, any further changes can come later. Feel free to merge.

@dmontagu dmontagu merged commit 7dcf502 into pydantic:main Feb 9, 2023
@dmontagu dmontagu deleted the add-core-schema-type-literal branch February 9, 2023 22:10
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.

4 participants