Skip to content

Make detection of TypeVar defaults robust to the CPython PEP-696 implementation#9426

Merged
sydney-runkle merged 1 commit intopydantic:mainfrom
AlexWaygood:tvar-default-fix
May 14, 2024
Merged

Make detection of TypeVar defaults robust to the CPython PEP-696 implementation#9426
sydney-runkle merged 1 commit intopydantic:mainfrom
AlexWaygood:tvar-default-fix

Conversation

@AlexWaygood
Copy link
Copy Markdown
Contributor

@AlexWaygood AlexWaygood commented May 11, 2024 •
edited by pydantic-hooky Bot
Loading

Change Summary

When implementing PEP 696 (default values for TypeVars, ParamSpecs and TypeVarTuples, we decided to implement the PEP slightly differently from the prototype that existed in typing_extensions. Specifically:

  • With the old implementation in typing_extensions:
    • If you passed default=None when constructing a TypeVar, the __default__ attribute would be set to types.NoneType
    • If you did not pass a value to the default= parameter, the __default__ attribute would be set to None
  • With the new implementation in CPython:
    • If you pass default=None when constructing a TypeVar, the __default__ attribute is set to None
    • If you do not pass a value to the default= parameter, the __default__ attribute is set to the new sentinel object typing.NoDefault.
  • The new implementation at CPython also has added a new has_default() method to TypeVars, ParamSpecs and TypeVarTuples

I backported these changes to the typing_extensions implementation of PEP 696 yesterday, and we realised today that it breaks some tests over at pydantic: python/typing_extensions#381. This PR fixes the tests so that they work if you're using a released version of typing_extensions, but they also work with the CPython implementation of PEP 696 and with the typing_extensions main branch.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@github-actions github-actions Bot added the relnotes-fix Used for bugfixes. label May 11, 2024
@AlexWaygood AlexWaygood mentioned this pull request May 11, 2024
Closed
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 11, 2024

CodSpeed Performance Report

Merging #9426 will not alter performance

Comparing AlexWaygood:tvar-default-fix (862ebe7) with main (7061f36)

Summary

✅ 13 untouched benchmarks

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 11, 2024

I tested this locally by making this change to pyproject.toml:

--- a/pyproject.toml
+++ b/pyproject.toml
@@ -46,7 +46,7 @@ classifiers = [
 ]
 requires-python = '>=3.8'
 dependencies = [
-    'typing-extensions>=4.6.1',
+    'typing-extensions @ git+https://github.com/python/typing_extensions.git',
     'annotated-types>=0.4.0',
     "pydantic-core==2.18.2",
 ]

And then rerunning make install, then make. The tests continued to pass using the main branch of typing_extensions.

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 11, 2024

please review

JelleZijlstra
JelleZijlstra approved these changes May 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

This looks correct to me. The try branch would be taken on Python 3.13+ and future versions of typing-extensions.

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 13, 2024

This PR should fix the "test typing-extensions main" job in pydantic's CI, which is currently failing on your main branch (e.g. see the test failures on #9431)

@antoni-jamiolkowski antoni-jamiolkowski mentioned this pull request May 14, 2024
Merged
5 tasks
sydney-runkle
sydney-runkle approved these changes May 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

@AlexWaygood,

Thanks so much for this fix! @JelleZijlstra, thanks a bunch for the review!

@sydney-runkle
Copy link
Copy Markdown
Contributor

sydney-runkle commented May 14, 2024

@AlexWaygood,

Before we merge, could you please add some tests for the behavioral changes you've listed above? Thanks again!

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 14, 2024 •
edited
Loading

@AlexWaygood,

Before we merge, could you please add some tests for the behavioral changes you've listed above? Thanks again!

Sure -- what kind of tests are you looking for? This fixes the failing CI job on main that tests with the typing_extensions main branch. So I think you already have coverage for the relevant code paths, or those tests wouldn't be failing. Should I add another CI job that explicitly tests with typing_extensions < 4.12.0, to make sure that those tests continue to pass with older versions of typing_extensions also installed?

@sydney-runkle
Copy link
Copy Markdown
Contributor

sydney-runkle commented May 14, 2024

@AlexWaygood,

That sounds great re the additional CI job. Thank you!

@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 14, 2024 •
edited
Loading

Oh, but it looks like you already have a CI job for testing with your oldest supported version of typing_extensions, as well as a CI job for testing with the typing_extensions main branch:

pydantic/.github/workflows/ci.yml

Lines 319 to 327 in 7061f36

test-typing-extensions:
name: test typing-extensions ${{ matrix.typing-extensions-version }} on Python ${{ matrix.python-version }}
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
# test the oldest supported version and main
typing-extensions-version: ['4.6.1', 'main']
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12']

So I'm not sure what I could add here @sydney-runkle. It seems like these code paths already have good test coverage (if they didn't, your CI job for testing with the typing_extensions main branch wouldn't be failing on your main branch). And it seems that you already check in CI that the tests pass with your oldest supported version of typing_extensions and with the typing_extensions main branch :-)

@AlexWaygood AlexWaygood mentioned this pull request May 14, 2024
Merged
@sydney-runkle
Copy link
Copy Markdown
Contributor

sydney-runkle commented May 14, 2024

@AlexWaygood,

Great point. I'm happy to merge this then. Thanks for the awesome work here!

@sydney-runkle sydney-runkle merged commit 86025fc into pydantic:main May 14, 2024
@AlexWaygood AlexWaygood deleted the tvar-default-fix branch May 14, 2024 18:30
@AlexWaygood
Copy link
Copy Markdown
Contributor Author

AlexWaygood commented May 14, 2024

Brilliant -- thanks @sydney-runkle! :D

@strangemonad strangemonad mentioned this pull request May 25, 2024
Closed
1 task
sydney-runkle pushed a commit that referenced this pull request May 28, 2024
@AlexWaygood @sydney-runkle
Make detection of TypeVar defaults robust to the CPython PEP-696 impl…
89d2d12 
…ementation (#9426)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@sydney-runkle sydney-runkle sydney-runkle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sydney-runkle sydney-runkle

Labels

ready for review relnotes-fix Used for bugfixes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexWaygood @sydney-runkle @JelleZijlstra