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

Update GMSO to work with pydantic 2.0 #745

Merged
merged 30 commits into from
Dec 13, 2023

Conversation

daico007
Copy link
Member

Pydantic v2 just recently comes out and seem to offer a lot of benefits, but also bring along many breaking changes to our current API. This PR will attempt to update GMSO classes to work with the latest version of pydantic.

gmso/abc/gmso_base.py Fixed Show fixed Hide fixed
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

gmso/abc/abstract_potential.py Fixed Show fixed Hide fixed
gmso/abc/abstract_site.py Fixed Show fixed Hide fixed
gmso/core/angle_type.py Fixed Show fixed Hide fixed
gmso/core/atom_type.py Fixed Show fixed Hide fixed
gmso/core/bond_type.py Fixed Show fixed Hide fixed
gmso/core/dihedral_type.py Fixed Show fixed Hide fixed
gmso/core/improper_type.py Fixed Show fixed Hide fixed
gmso/core/pairpotential_type.py Fixed Show fixed Hide fixed
gmso/core/parametric_potential.py Fixed Show fixed Hide fixed
gmso/abc/abstract_site.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c014a44) 92.81% compared to head (e69f03b) 92.36%.

Files Patch % Lines
gmso/core/atom.py 92.00% 2 Missing ⚠️
gmso/core/atom_type.py 91.66% 2 Missing ⚠️
gmso/core/element.py 88.88% 1 Missing ⚠️
gmso/formats/json.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
- Coverage   92.81%   92.36%   -0.46%     
==========================================
  Files          66       66              
  Lines        6862     6860       -2     
==========================================
- Hits         6369     6336      -33     
- Misses        493      524      +31     

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

@daico007 daico007 requested a review from CalCraven November 25, 2023 03:17
Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

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

So what is the result of removing the JSONHandler class in gmso.abc.serialization_utils? Is that going to change the ability to use the top.save("top.json") method?

Otherwise, minus just some questions for my understanding this is good to go.

gmso/lib/potential_templates.py Outdated Show resolved Hide resolved
gmso/lib/potential_templates.py Outdated Show resolved Hide resolved
@daico007
Copy link
Member Author

The reason JSONHandler got removed is because pydantic decided to deprecate the ability to have our own Json handler altogether (we used it before to handle non-serializable property, like unyt and sympy expression). The new way they try to make people do is having the custom json writer determined for each class (this is part of the work in this PR). The JSON reader and writer still works (the unit tests for those are kept mostly intact).

@CalCraven
Copy link
Contributor

Excellent, good to know. I'm about to push fixes to these conflicts with the main branch, then we can get this merged.

@daico007 daico007 merged commit 096da71 into mosdef-hub:main Dec 13, 2023
13 of 14 checks passed
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.

2 participants