openff-toolkit
openff-toolkit copied to clipboard
Disallow `ToolkitWrapper` objects passed to toolkit_registry
In #1050 @j-wags and I re-encountered strange behavior relating to the toolkit_registry argument in some Molecule API points. The behavior is different if a ToolkitWrapper and ToolkitRegistry argument is passed and the wrapper or all wrappers in the registry fail. I can expand on the failure cases later.
A solution would involve forbidding ToolkitWrapper objects passed to toolkit_registry; initially with a deprecation warning but later with an exception. The migration would involve simply passing a single-wrapper registry in place of a wrapper object, i.e.
Molecule.some_api_point(
...
toolkit_registry=OpenEyeToolkitWrapper(),
...
)
will need to become
Molecule.some_api_point(
...
toolkit_registry=ToolkitRegistry(OpenEyeToolkitWrapper()),
...
)
Could you just do that under the hood -- check if a wrapper is passed in, and if so, make a single-wrapper registry?
Agreed, another solution would be to convert the wrappers for the user. This would be backwards-compatible at a high-level but would introduce some behavior changes, i.e. a specific exception that an individual wrapper raises upon the failure of one of its methods (like ChargeAssignmentError or something like that) would become the ValueError resulting from a registry failing.
It's a tricky decision point since these methods are all accessible from the wrapper and registry objects themselves and these parts of the Molecule API are "just" convenience for the users - but super convenient, and also some of our most commonly-used API points!
This won't happen - but #1495 does what I think Lily suggested as a clean workaround.