openff-toolkit icon indicating copy to clipboard operation
openff-toolkit copied to clipboard

Disallow `ToolkitWrapper` objects passed to toolkit_registry

Open mattwthompson opened this issue 4 years ago • 2 comments

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()),
    ...
)

mattwthompson avatar Sep 08 '21 22:09 mattwthompson

Could you just do that under the hood -- check if a wrapper is passed in, and if so, make a single-wrapper registry?

lilyminium avatar Sep 08 '21 22:09 lilyminium

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!

mattwthompson avatar Sep 08 '21 22:09 mattwthompson

This won't happen - but #1495 does what I think Lily suggested as a clean workaround.

mattwthompson avatar Jan 27 '23 20:01 mattwthompson