Skip to content

Fix testing infrasucture for Pydantic 2 default#2041

Merged
pkalita-lbl merged 6 commits intomainfrom
issue-2040-pydantic-1-testing
Apr 2, 2024
Merged

Fix testing infrasucture for Pydantic 2 default#2041
pkalita-lbl merged 6 commits intomainfrom
issue-2040-pydantic-1-testing

Conversation

@pkalita-lbl
Copy link
Copy Markdown
Contributor

@pkalita-lbl pkalita-lbl commented Mar 29, 2024

Fixes #2040

Summary

  • With Pydandic 2.x.x specified in our poetry.lock file, running tests directly via pytest or the tox py3x testenvs will use Pydantic v2. These changes replace the existing "special case" pydantic2 testenv with a pydantic1 testenv. The new testenv operates the same as the old one, except swapping Pydantic v1 for v2.
  • Previously the main.yaml testing workflow had a pydantic-version variable in the configuration matrix. With the addition of Python 3.11 and 3.12 to the testing matrix (Add support for Python 3.12 #1704), the matrix permutations were starting to get a little unruly. Here I'm splitting off Pydantic 1 testing into a separate workflow (.github/workflows/test-pydantic1.yaml). I then updated the exclude combinations in main.yaml so that what we end up running is:
    • All supported Python version with Pydantic 2 on Ubuntu (main.yaml)
    • The oldest and newest supported Python versions with Pydantic 2 on Windows (main.yaml)
    • The latest supported Python version with Pydantic 1 on Ubuntu (test-pydantic1.yaml)
  • The updated testing workflows caught a few issues we had missed with our previous configuration matrix. @sneakers-the-rat fixed a few issues in the Pydantic generator that crop up when using older Python versions. I updated a few compliance tests to accurately reflect the actual behavior of Pydantic on older Python versions.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 80.64%. Comparing base (8a28750) to head (b5d0fb8).
Report is 1 commits behind head on main.

Files Patch % Lines
linkml/generators/pydanticgen/array.py 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2041      +/-   ##
==========================================
- Coverage   80.67%   80.64%   -0.03%     
==========================================
  Files         107      107              
  Lines       11943    11946       +3     
  Branches     3415     3416       +1     
==========================================
- Hits         9635     9634       -1     
- Misses       1743     1744       +1     
- Partials      565      568       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkalita-lbl
Copy link
Copy Markdown
Contributor Author

Notes to self: seems like we've never actually tested the output of the Pydantic generator, targeting v2, on Python 3.8 or 3.9. The 3.8 tests are failing with a lot of

ImportError: cannot import name 'Annotated' from 'typing'

Makes sense, because that was introduced in Python 3.9 (https://docs.python.org/3/library/typing.html#typing.Annotated)

On 3.9 the errors are mostly

AttributeError: '_SpecialForm' object has no attribute '__name__'

They also seem to be related to array test cases. Needs further investigation.

@sneakers-the-rat
Copy link
Copy Markdown
Collaborator

oop how did i miss that. assigning myself bc these are my fault probs

@sneakers-the-rat sneakers-the-rat self-assigned this Mar 31, 2024
…dels with AnyShapeArray

Hardcode Any name check since it's a special case before 3.11
@sneakers-the-rat
Copy link
Copy Markdown
Collaborator

sneakers-the-rat commented Mar 31, 2024

fixed the array generation and version problems. unassigning myself bc that was the 'my fault' part.

the remaining test failures look like they're from pydantic coercion, where a model like

class C(BaseModel):
    s1: Optional[bool] = None

incorrectly validates with

inst = C(s1=1.1)

as

C(s1=True)

which is a pydantic coercion thing. idk what would be the desired behavior there - we could enable strict mode ( #1955 ) or we could mark the expected behavior as COERCES. i'll defer to y'all on what the fix should be.

weird that it's happening only in older python versions though, don't know why that would be.
edit: even weirder, only on older versions in linux, the windows tests pass.

@sneakers-the-rat sneakers-the-rat removed their assignment Mar 31, 2024
@pkalita-lbl
Copy link
Copy Markdown
Contributor Author

Much appreciated @sneakers-the-rat! I will take care of the last few failures.

@pkalita-lbl
Copy link
Copy Markdown
Contributor Author

I think our best move here is to just mark those narrows cases in the compliance tests as having coerce as the expected behavior. I didn't find any issues in the Pydantic repo that directly address what we're seeing on Python < 3.10 even though it directly contradicts what they say in their docs.

The error is coming from Pydantic's Rust core. Because of that I'm not totally surprised that the issue seems to only effect certain Python versions on Ubuntu and not Windows.

@pkalita-lbl pkalita-lbl marked this pull request as ready for review April 2, 2024 20:00
pydantic-version: "2"
python-version: "3.10"
- os: windows-latest
python-version: "3.11"
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.

not 3.12 right? (pydantic1 seems to be setting up a 3.12 environment)

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.

Right. These are excludes, and when we subtract them from the default permutations they leave us with only Python 3.8 and 3.12 on Windows.

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.

I see, I missed the excludes part.

- `network` - (currently unused in CI) - tests marked as requiring network access/making network requests in order to succeed.
- `slow` - tests that are necessary to test technical correctness but are sufficiently long that it's worth excluding them during routine development/when running tests locally.
- `network` - (currently unused in CI) - tests marked as requiring network access/making network requests in order to succeed.
- `slow` - tests that are necessary to test technical correctness but are sufficiently long that it's worth excluding them during routine development/when running tests locally.
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.

thank you for this doc.

@pkalita-lbl pkalita-lbl merged commit 6237984 into main Apr 2, 2024
@pkalita-lbl pkalita-lbl deleted the issue-2040-pydantic-1-testing branch April 2, 2024 21:20
@sneakers-the-rat
Copy link
Copy Markdown
Collaborator

Go team!!!

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.

Update tox.ini and github CI to make Pydantic 1 the special test scenario

3 participants