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

IR: Update to Pydantic >2.0 compatibility #349

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Jul 22, 2024

Note:` For now, just for testing...

This PR brings a few enabling changes to the internal IR node definitions that allow us to lift the version restriction on pydantic. We needed this previously, as we were relying on some (broken) default behaviour that was fixed upstream in pydantic version 2. By changing to the correct type annotations, in particular for Optional arguments, we can now lift this restriction.

In addition, I also added a small set of rudimentary checks for the constructor and object behaviour (immutability) for three most common/interesting node types. I plan to expand on this somewhat in a follow-up PR that will require pydantic features from version 2.x.

In some more detail:

  • Use Optional throughout node type annotations to allow default values of None
  • Add a rudimentary test for basic constructor behaviour and "frozeness" of our IR nodes, for Assignment, Loop and Conditional
  • Lift version restriction on pydantic and add a few small fixes to deal with the fallout

Note: This is the first of two PRs and paves the way for more cool stuff to come (as discussed offline).

Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/349/index.html

@mlange05 mlange05 force-pushed the naml-pydantic-update branch 2 times, most recently from c2ff8e1 to fe4656f Compare July 23, 2024 03:42
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.30%. Comparing base (78e7605) to head (07d89ec).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   95.29%   95.30%   +0.01%     
==========================================
  Files         170      171       +1     
  Lines       36414    36490      +76     
==========================================
+ Hits        34701    34778      +77     
+ Misses       1713     1712       -1     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 95.28% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

The `validate_assignments` config options was negating the
"frozenness" of the dataclasses in the newer pydantic releases.
@mlange05 mlange05 requested a review from reuterbal July 23, 2024 18:03
@mlange05 mlange05 marked this pull request as ready for review July 23, 2024 18:03
Copy link
Collaborator

@reuterbal reuterbal 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 great, many thanks for grinding this through!

@reuterbal reuterbal added the ready for merge This PR has been approved and is ready to be merged label Jul 24, 2024
@reuterbal reuterbal merged commit 536908b into main Jul 24, 2024
12 checks passed
@reuterbal reuterbal deleted the naml-pydantic-update branch July 24, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants