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

Add deregister_toolkit method #614

Merged
merged 13 commits into from Jul 8, 2020
Merged

Add deregister_toolkit method #614

merged 13 commits into from Jul 8, 2020

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jun 3, 2020

#468

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2020

This pull request introduces 2 alerts and fixes 1 when merging 3f3be8a into 97d31e6 - view on LGTM.com

new alerts:

  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Unused import

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020

Codecov Report

Merging #614 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

@dotsdl dotsdl requested review from dotsdl and removed request for dotsdl June 9, 2020 16:13
@lgtm-com
Copy link

lgtm-com bot commented Jun 26, 2020

This pull request introduces 2 alerts and fixes 1 when merging 5365cf9 into 131d86c - view on LGTM.com

new alerts:

  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Module is imported more than once

@mattwthompson
Copy link
Member Author

I bumped this up in priority since it's been desired multiple times in recent memory. I've merged in the other 0.7.0 changes, updated tests and docstrings, and also catch the case of attempting to deregister an un-registered toolkit. I believe this is ready for review now.

I've completely punted on retooling GLOBAL_TOOLKIT_REGISTRY since I want to get this feature out sooner and the two changes should be mostly de-coupled from each other.

Tangentially, the toolkit wrappers could probably do with a __str__ or __repr__ override to make things like this a little prettier:

In [4]: registry = ToolkitRegistry(toolkit_precedence=[AmberToolsToolkitWrapper])

In [5]: registry.deregister_toolkit(RDKitToolkitWrapper)
---------------------------------------------------------------------------
ToolkitUnavailableException               Traceback (most recent call last)
<ipython-input-5-0e9acceb2423> in <module>
----> 1 registry.deregister_toolkit(RDKitToolkitWrapper)

~/software/openforcefield/openforcefield/utils/toolkits.py in deregister_toolkit(self, toolkit_wrapper)
   4348                     f"Currently registered toolkits are {self._toolkits}"
   4349                 )
-> 4350                 raise ToolkitUnavailableException(msg)
   4351
   4352         for toolkit_to_remove in toolkits_to_remove:

ToolkitUnavailableException: Did not find <openforcefield.utils.toolkits.RDKitToolkitWrapper object at 0x129036750> in registry. Currently registered toolkits are [<openforcefield.utils.toolkits.AmberToolsToolkitWrapper object at 0x111610990>]

In [6]: registry.registered_toolkits
Out[6]: [<openforcefield.utils.toolkits.AmberToolsToolkitWrapper at 0x111610990>]

I could probably sneak that in here - but when I say that, it typically works to not be true after trying it.

@lgtm-com
Copy link

lgtm-com bot commented Jun 26, 2020

This pull request introduces 2 alerts and fixes 1 when merging 45ab3da into 131d86c - view on LGTM.com

new alerts:

  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Module is imported more than once

@lgtm-com
Copy link

lgtm-com bot commented Jun 26, 2020

This pull request introduces 2 alerts and fixes 1 when merging 3fa8d0f into 131d86c - view on LGTM.com

new alerts:

  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Module is imported more than once

@lgtm-com
Copy link

lgtm-com bot commented Jul 7, 2020

This pull request introduces 2 alerts and fixes 1 when merging d78e111 into e1b6e74 - view on LGTM.com

new alerts:

  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Module is imported more than once

Copy link
Member

@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.

Nice tests -- Very clever and thorough. Made a few direct suggestions, and there's one docstring that seems to have wandered off, but after that I think this is good to merge.

Thanks, Matt!

openforcefield/tests/test_toolkits.py Outdated Show resolved Hide resolved
openforcefield/tests/test_toolkits.py Outdated Show resolved Hide resolved
openforcefield/tests/test_toolkits.py Outdated Show resolved Hide resolved
openforcefield/tests/test_toolkits.py Show resolved Hide resolved
with pytest.raises(ToolkitUnavailableException):
toolkit_registry.deregister_toolkit(RDKitToolkitWrapper)

def deregister_from_global_registry(self):
Copy link
Member

Choose a reason for hiding this comment

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

Yup. This is a pretty weird one. Hopefully this can be made more sensical when we move to a singleton design pattern or context manager approach.

openforcefield/utils/toolkits.py Outdated Show resolved Hide resolved
"""
if not isinstance(toolkit_wrapper, ToolkitWrapper):
msg = "Something other than a ToolkitWrapper object was passed to ToolkitRegistry.add_toolkit()\n"
msg += "Given object {} of type {}".format(toolkit_wrapper,
type(toolkit_wrapper))
raise Exception(msg)
raise InvalidToolkitError(msg)
Copy link
Member

Choose a reason for hiding this comment

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

Hooray for specific exceptions!

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.

None yet

3 participants