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

Test new openff-models w/ pydantic v2 #336

Closed
wants to merge 12 commits into from
Closed

Test new openff-models w/ pydantic v2 #336

wants to merge 12 commits into from

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Aug 21, 2024

No description provided.

@mikemhenry
Copy link
Contributor

I am sure that pip check is exposing a real error, but for the sake of testing this I would comment it out.

@IAlibay
Copy link
Member Author

IAlibay commented Aug 21, 2024

Known errors:

  1. openff.units needs Kelvin = kelvin
  2. some issue with comparing frozen vs unfrozen comparison

@IAlibay
Copy link
Member Author

IAlibay commented Aug 21, 2024

Some thing about GUFE key stability

@IAlibay
Copy link
Member Author

IAlibay commented Aug 21, 2024

Calling schema_json is also failing.

IAlibay and others added 2 commits August 21, 2024 17:37
@pep8speaks
Copy link

pep8speaks commented Aug 21, 2024

Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 19:1: E265 block comment should start with '# '
Line 45:5: E265 block comment should start with '# '
Line 104:9: E265 block comment should start with '# '
Line 105:9: E265 block comment should start with '# '
Line 109:9: E265 block comment should start with '# '
Line 112:9: E265 block comment should start with '# '
Line 112:9: E303 too many blank lines (2)

Comment last updated at 2024-08-27 11:43:27 UTC

@mattwthompson
Copy link
Contributor

Any other of these classes you'd like? For example, I'm probably adding _ElementaryChargeQuantity https://github.com/openforcefield/openff-interchange/pull/1066/files#diff-e701185426285c406ac35c83d090e6784f044d92574a9f9527078b20f122f649R158-R164

@mattwthompson
Copy link
Contributor

mattwthompson commented Oct 16, 2024

^ can still add whatever classes, or more usefully document how to generate them downstream

Currently I'm planning to roll with these changes being in Interchange, not a release of openff-models

@IAlibay
Copy link
Member Author

IAlibay commented Oct 16, 2024

Currently I'm planning to roll with these changes being in Interchange, not a release of openff-models

@mattwthompson can you maybe provide a bit more detail on what you mean by this? I.e. is openff-models not being updated anymore or do you mean the addition of these extra classes are only happening in Interchange?

openff-models going away completely is going to be.. a bit of an issue for us, we definitely can't have gufe depend on a big dependency like Interchange.

@mattwthompson
Copy link
Contributor

Interchange 0.4.0 doesn't depend on openff-models, it just has the relevant bits ported over into one or two files since the functionality is pretty small in scope. It can be accessed with openff-interchange-base which is pretty lightweight or just ported over here.

@dotsdl dotsdl self-requested a review October 22, 2024 16:18
@IAlibay
Copy link
Member Author

IAlibay commented Oct 22, 2024

Question from today's alchemiscale: should the default strategy be that we vendor openff-models? I.e. is openff-models going to update again or should we go with what's in interchange?

cc @mattwthompson

@mattwthompson
Copy link
Contributor

Let me try vendoring it here to see if it's as easy as I suspect. Should I work off of this branch or a different reference?

@IAlibay
Copy link
Member Author

IAlibay commented Oct 22, 2024

Let me try vendoring it here to see if it's as easy as I suspect. Should I work off of this branch or a different reference?

I think for now I would say "don't put any extra work on this" - if we can work out what OpenFF's intended future is, then we can explore the idea of "should we vendor".

Read: "I don't want to create any extra work for anyone".

@mattwthompson
Copy link
Contributor

mattwthompson commented Oct 22, 2024

I don't have plans to make a new release of openff-models and I don't think anything else in OpenFF needs the update; the functionality we need is now in openff/interchange/_annotations.py and possibly a few other locations.

The relevant files are probably around these parts:

Some complexity can be stripped out if unyt and/or openmm.unit support isn't needed, and I think a couple of "deprecated" features might still be used.

@mattwthompson
Copy link
Contributor

Of course, you could also just try not updating at all and see what happens

@IAlibay
Copy link
Member Author

IAlibay commented Oct 24, 2024

Thanks @mattwthompson - @dotsdl did you want to discuss this further? We probably should work out a) if we need to implement the changes, and b) how we go about it.

@dotsdl
Copy link
Member

dotsdl commented Nov 12, 2024

We decided that we will proceed with vendoring the components relevant to gufe from openff-models and go from there. I've created this issue for that activity: #397

I think we can close this PR; thank you @mattwthompson for your help with this!

@IAlibay IAlibay closed this Nov 12, 2024
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.

5 participants