openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
301 stars 88 forks source link

`unit` argument to `ParameterAttribute` not sanitized #1888

Closed mattwthompson closed 2 weeks ago

mattwthompson commented 3 weeks ago

Describe the bug

I'm working on a isolating something from a more involved example, but for now I can demonstrate that this argument is not checked for proper types

https://github.com/openforcefield/openff-toolkit/blob/fdf383261717a1ca9fe72586e2463a425c04e0f9/openff/toolkit/typing/engines/smirnoff/parameters.py#L342-L350

which later causes this clause to bubble up in confusing ways - below because self._unit is allowed to be a string, it bubbles up a confusing error that nanometers aren't compatible with nanometers.

https://github.com/openforcefield/openff-toolkit/blob/fdf383261717a1ca9fe72586e2463a425c04e0f9/openff/toolkit/typing/engines/smirnoff/parameters.py#L416-L420

It's possible I'm missing something with how stuff is passed through these classes, but it's hard for me to figure out from the code itself.

To Reproduce

Passing a string to the unit argument is bad, but for now just consider the possibly that something could get botched and let it through like that or that a user would give it a try.

In [1]: from openff.toolkit.typing.engines.smirnoff.parameters import (
   ...:     ParameterAttribute,
   ...:     unit,
   ...:     Quantity,
   ...: )

In [2]: parameter_attribute = ParameterAttribute(
   ...:     default=Quantity("1 nanometer"),
   ...:     unit="nanometer",
   ...: )

In [3]: parameter_attribute._name = "_foo"  # Not relevant to this reproduction

In [4]: parameter_attribute._validate_units(value=Quantity("2 nanometer"))
/Users/mattthompson/micromamba/envs/new-models/lib/python3.12/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:585: SyntaxWarning: invalid escape sequence '\ '
  """The attribute of a parameter with an unspecified number of terms, where
/Users/mattthompson/micromamba/envs/new-models/lib/python3.12/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:1614: SyntaxWarning: invalid escape sequence '\ '
  """
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
File ~/micromamba/envs/new-models/lib/python3.12/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:412, in ParameterAttribute._validate_units(self, value)
    411 try:
--> 412     if not self._unit.is_compatible_with(value.units):
    413         raise IncompatibleUnitError(
    414             f"{self.name}={value} should have units of {self._unit}"
    415         )

AttributeError: 'str' object has no attribute 'is_compatible_with'

During handling of the above exception, another exception occurred:

IncompatibleUnitError                     Traceback (most recent call last)
Cell In[4], line 1
----> 1 parameter_attribute._validate_units(value=Quantity("2 nanometer"))

File ~/micromamba/envs/new-models/lib/python3.12/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:418, in ParameterAttribute._validate_units(self, value)
    413             raise IncompatibleUnitError(
    414                 f"{self.name}={value} should have units of {self._unit}"
    415             )
    416     except AttributeError:
    417         # This is not a Quantity object.
--> 418         raise IncompatibleUnitError(
    419             f"{self.name}={value} should have units of {self._unit}"
    420         )
    421 return value

IncompatibleUnitError: foo=2 nanometer should have units of nanometer
mattwthompson commented 2 weeks ago

Just to be clear, there are two potential issues here:

  1. (What I focused on above) that the unit argument is not sanitized in a way that's consistent with the type annotation
  2. (A separate, more involved use case that I pair-programmed with @j-wags on) a mysterious possibility that foo = ParameterAttribute(default=None, unit=unit.nanometer) is magically stringified in some of the machinery, causing problems later on that look similar to what happens in some cases with (1)

1890 proposes one of two ways of dealing with (1), and I think either would be an improvement in a vacuum

(2) I can't actually reproduce with a full head of steam; it looks like something was funky on my local machine with a combination of editable builds on top of editable builds and virtual site non-bonded interactions shadowing atomistic non-bonded interactions of the same name, one having accidentally done a ..., unit="nanometer" type deal.