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 ToolkitRegistry.deregister_toolkit() to make it easier to remove toolkits from the global toolkit registry? #468

Closed
jchodera opened this issue Nov 30, 2019 · 6 comments

Comments

@jchodera
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently, the easiest way to remove the OpenEye toolkit from the global toolkit registry is via mucking about with private fields:

from openforcefield.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY
GLOBAL_TOOLKIT_REGISTRY._toolkits = GLOBAL_TOOLKIT_REGISTRY._toolkits[1:]

Describe the solution you'd like
Instead, we might want to add a deregister_toolkit() method:

from openforcefield.utils.toolkits import GLOBAL_TOOLKIT_REGISTRY
openeye_toolkit = GLOBAL_TOOLKIT_REGISTRY.registered_toolkits[0]
GLOBAL_TOOLKIT_REGISTRY.deregister_toolkit(openeye_toolkit)

Describe alternatives you've considered
We could alternatively extend the registered_toolkits field to behave more like a list, supporting methods such as GLOBAL_TOOLKIT_REGISTRY.registered_toolkits.pop(0).

Alternatively, we could just change the private field ._toolkits to registered_toolkits and allow users to manipulate this field directly if there is no reason to keep it private (which there seems not to be at the moment).

@j-wags
Copy link
Member

j-wags commented Dec 17, 2019

Agree with either API suggestion. Treating it like a list might be nice, since then users could rearrange toolkit priority in-place. If we go with the list route, I'd like to add a lot of testing, to make sure that the range of list methods like del, pop, insert, replace, etc have sane behavior or raise informative errors.

@jchodera
Copy link
Member Author

Good point. This documentation may be relevant.

@jchodera
Copy link
Member Author

jchodera commented Jun 1, 2020

Bumping this since this issue makes it difficult to debug user issues with non-default backends.

cc: #611 (comment)

@dotsdl
Copy link
Member

dotsdl commented Jun 1, 2020

Acknowledged! Added this to our software scientist kanban for dev time allocation.

@mattwthompson
Copy link
Member

After #614, toolkit wrappers can be deregistered from either the global registry or a custom registry (sample below). There may be other complications with how GLOBAL_TOOLKIT_REGISTRY which can be discussed using #493 as a stub or a new issue

In [1]: from openforcefield.utils.toolkits import *

In [2]: GLOBAL_TOOLKIT_REGISTRY.registered_toolkits
Out[2]:
[<openforcefield.utils.toolkits.OpenEyeToolkitWrapper at 0x11049bdd0>,
 <openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x11049bfd0>,
 <openforcefield.utils.toolkits.AmberToolsToolkitWrapper at 0x1104ae710>,
 <openforcefield.utils.toolkits.BuiltInToolkitWrapper at 0x11c14e510>]

In [3]: GLOBAL_TOOLKIT_REGISTRY.deregister_toolkit(OpenEyeToolkitWrapper)

In [4]: GLOBAL_TOOLKIT_REGISTRY.registered_toolkits
Out[4]:
[<openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x11049bfd0>,
 <openforcefield.utils.toolkits.AmberToolsToolkitWrapper at 0x1104ae710>,
 <openforcefield.utils.toolkits.BuiltInToolkitWrapper at 0x11c14e510>]

In [5]: registry = ToolkitRegistry(toolkit_precedence=[OpenEyeToolkitWrapper, RDKitToolkitWrapper])

In [6]: registry.registered_toolkits
Out[6]:
[<openforcefield.utils.toolkits.OpenEyeToolkitWrapper at 0x11f6462d0>,
 <openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x11f646d10>]

In [7]: registry.deregister_toolkit(OpenEyeToolkitWrapper)

In [8]: registry.registered_toolkits
Out[8]: [<openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x11f646d10>]

@j-wags
Copy link
Member

j-wags commented Aug 26, 2020

Closing, since this specific issue was resolved by #614. Discussions on the singleton-ness of GLOBAL_TOOLKIT_REGISTRY are continuing in #493

@j-wags j-wags closed this as completed Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants