-
Notifications
You must be signed in to change notification settings - Fork 91
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
Allow Pydantic v2 in test environments #1684
base: main
Are you sure you want to change the base?
Conversation
mattwthompson
commented
Aug 4, 2023
- Tag issue being addressed
- Add tests
- Update docstrings/documentation, if applicable
- Lint codebase
- Update changelog
Will wait to finish this until |
f9b7f81
to
b6db523
Compare
Seems to be working, modulo QCArchive allowing records to be pulled down |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems to do 6 things:
- Adds pydantic=1 and 2 variants to the CI matrix
- (maybe) Changes some things to make pydantic 2 work
- Changes some things to make mypy happy
- Has pre-commit start removing blank lines at ends of files
- Makes some helpful-looking changes to
Molecule.visualize
tests - Gets
has_pkg
andrequires_pkg
from openff-utils instead of keeping a redundant definition
For 1 - The changes here seem to have made the test configurations more complex/brittle. What form would the final CI changes take if this is merged?
For 2 - Are there any source code changes in here to get pydantic 2 compatibility or did we have that all along?
The changes for items 3, 4, 5, and 6 are good by me.
The toolkit does not use Pydantic at all; the objective here is to ensure tests pass with both v1 and v2. No code changes (here) are needed to allow users to install v2. The main complexity added here is working around QCArchive not supporting v2. I don't take dropping QCArchive interoperability or limiting Pydantic compatibility to be workable options, and as long as v1 is still supported at installation time it's something that should be tested. So the final matrix will look a little more complex, with maybe a couple of combinations stripped out. |
Gotcha. Ok, it's great to know that OFFTK is ready for pydantic v2. I'd like to leave this open until the pydantic v2-compatible QCF packages are out. If we merge it sooner, we have no choice but to expand the cost/complexity of our CI matrix, and then make a second round of changes once the new QCF packages come out. If we want to allow pydantic=2 in our conda packages before then, I'd approve a PR/patch to do so! |
Seems like we already do https://prefix.dev/channels/conda-forge/packages/openff-toolkit-base |