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

Do not use 'old' OpenFF Toolkit API #545

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

mattwthompson
Copy link
Member

PR Summary:

A change I made to the toolkit (openforcefield/openff-toolkit#1661) involved changing how attributes are looked up, which made this check

        uses_old_api = hasattr(Topology(), "_topology_molecules")

no longer produce the same behavior as when I wrote it. With 0.14.4, released last night, this line erroneously evaluates to True. There would be a few ways to fix this, but I'm proposing to just remove the dual support. We are no longer supporting the "old" (< 0.11) versions of the toolkit. Note, of course, that this change wouldn't affect any released version of Foyer.

I also noticed tests aren't run in parallel - it's a small and straightforward fix to get CI to use the multiple cores that GHA provides
. I'm lazy.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #545 (81085c5) into main (d2608b7) will increase coverage by 0.33%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #545      +/-   ##
==========================================
+ Coverage   69.21%   69.55%   +0.33%     
==========================================
  Files          16       16              
  Lines        1673     1662      -11     
==========================================
- Hits         1158     1156       -2     
+ Misses        515      506       -9     

mattwthompson added a commit to openforcefield/openff-interchange that referenced this pull request Oct 6, 2023
Replace when merged and in a release

mosdef-hub/foyer#545
Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

LGTM! Do we want to pin the openff-toolkit version also?

@mattwthompson
Copy link
Member Author

The only constraint is openff-toolkit >=0.11 but that's already in there. The quirk of 0.14.4 wasn't an API break so it should work all the way back to 0.11

@daico007 daico007 merged commit 60739cf into mosdef-hub:main Oct 8, 2023
14 checks passed
@mattwthompson
Copy link
Member Author

Thanks @daico007! Anything I can do to help a release? (If you have a checklist/process in place I'm happy to do as much of it as you are comfortable dumping off)

@daico007
Copy link
Member

daico007 commented Oct 8, 2023

No problem @mattwthompson. I will put a out a patch release for foyer Monday (I may attempt to fix the current Dockerfile before that but there's a good chance it fill fail again)

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