Skip to content

Update smirnoff_hack.py for OpenFF Toolkit 0.10.3#248

Closed
mattwthompson wants to merge 3 commits intoleeping:masterfrom
mattwthompson:openff-toolkit-0.10.3
Closed

Update smirnoff_hack.py for OpenFF Toolkit 0.10.3#248
mattwthompson wants to merge 3 commits intoleeping:masterfrom
mattwthompson:openff-toolkit-0.10.3

Conversation

@mattwthompson
Copy link
Copy Markdown
Contributor

@mattwthompson mattwthompson commented Mar 1, 2022

We released a new version of the toolkit yesterday that broke smirnoff_hack by nature of adding a named argument to generate_conformers - fixing it should be a matter of string the new argument through the appropriate functions.

@mattwthompson
Copy link
Copy Markdown
Contributor Author

CI seems to be failing for other reasons (pip and/or other deps changing). I might be able to have a look later.

Copy link
Copy Markdown
Collaborator

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Ahh gosh, sorry, I keep forgetting about this file. Yes, the OFF Toolkit 0.10.3 release yesterday added a kwarg to the generate_conformers method signature which broke the caching logic here. I'm sorry for the inconvenience @leeping.

This code change looks correct to me. @leeping - ForceBalance's CI hasn't run since the 0.10.3 OFF toolkit release, so I can't show you the problem, but if we run CI on master right now it should fail because of this problem.

Apologies again, I think our ask is both for this PR and for a ForceBalance release. There may be other issues happening too with other deps and we could discuss how to handle those as well.

@mattwthompson mattwthompson force-pushed the openff-toolkit-0.10.3 branch from 625df4d to 7bc9350 Compare March 1, 2022 22:30
@mattwthompson
Copy link
Copy Markdown
Contributor Author

I've updated this to be self-contained to the changes associated with the toolkit. I opened #249 in an effort to fix other failing tests. Hopefully that can be accomplished before this one.

@j-wags
Copy link
Copy Markdown
Collaborator

j-wags commented Mar 2, 2022

The approach from #231 (switching from explicit listing of args and kwargs to using *args and **kwargs) would probably be a more future-proof way of handling this.

In the long run it'd be great to fully move the caching out of forcebalance and into the toolkit. Maybe we could coordinate with @leeping on that so we can synchronize making that change and a subsequent release in the future? Then we could stabilize the FB interface and not cause this kind of problem in the future.

@mattwthompson
Copy link
Copy Markdown
Contributor Author

Agreed, I've opened #251 with a smaller set of changes

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