make sure we don't install the newest toolkit by mistake

[mikemhenry]

It looks like for the CI steps where we are not using the latest openff-toolkit, we actually are still pulling in 0.11. This PR should fix that and make sure we support both 0.11 and 0.10.x

[mikemhenry]

Example:

https://github.com/openmm/openmmforcefields/actions/runs/3065089613/jobs/4948832540

  openff-toolkit                 0.11.0        pyhd8ed1ab_1            conda-forge

[mikemhenry]

This line: https://github.com/openmm/openmmforcefields/blob/main/openmmforcefields/tests/test_template_generators.py#L71

Is giving us an error: E AttributeError: Neither Quantity object nor its magnitude (0.1) has attribute 'unit' https://github.com/openmm/openmmforcefields/actions/runs/3065768844/jobs/4950205125#step:9:76

[codecov-commenter]

Codecov Report

Merging #237 (80ed36f) into main (35496e5) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   76.66%   77.08%   +0.41%     
==========================================
  Files           5        5              
  Lines         840      851      +11     
==========================================
+ Hits          644      656      +12     
+ Misses        196      195       -1     

Impacted Files	Coverage Δ
...penmmforcefields/generators/template_generators.py	86.04% <100.00%> (+0.42%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mikemhenry]

A bit more context:

self = <Quantity(0.1, 'angstrom')>, item = 'unit'

    def __getattr__(self, item) -> Any:
        if item.startswith("__array_"):
            # Handle array protocol attributes other than `__array__`
            raise AttributeError(f"Array protocol attribute {item} not available.")
        elif item in HANDLED_UFUNCS or item in self._wrapped_numpy_methods:
            magnitude_as_duck_array = _to_magnitude(
                self._magnitude, force_ndarray_like=True
            )
            try:
                attr = getattr(magnitude_as_duck_array, item)
                return functools.partial(self._numpy_method_wrap, attr)
            except AttributeError:
                raise AttributeError(
                    f"NumPy method {item} not available on {type(magnitude_as_duck_array)}"
                )
            except TypeError as exc:
                if "not callable" in str(exc):
                    raise AttributeError(
                        f"NumPy method {item} not callable on {type(magnitude_as_duck_array)}"
                    )
                else:
                    raise exc
    
        try:
            return getattr(self._magnitude, item)
        except AttributeError:
>           raise AttributeError(
                "Neither Quantity object nor its magnitude ({}) "
                "has attribute '{}'".format(self._magnitude, item)
            )
E           AttributeError: Neither Quantity object nor its magnitude (0.1) has attribute 'unit'

[mikemhenry]

@mattwthompson I'm not sure why this is failing...

[mikemhenry]

So this kinda made things better molecule.conformers[0][0,0] += to_openmm(0.1*unit.angstroms)

But then later I get a failure do to this bit of code:

if type(net_charge) != unit.Quantity:
    # openforcefield toolkit < 0.7.0 did not return unit-bearing quantity
    # how long should openmmforcefields support < 0.7.0?
    net_charge = float(net_charge) * unit.elementary_charge

Which we don't really need anymore, but getting rid of that gives us yet another new error:

self = Quantity(value=array([ 0.5627, -0.5151, -0.0238, -0.0238]), unit=elementary charge)
other = Quantity(value=array([6.93889390e-18, 6.35191798e-18, 2.93487960e-19, 2.93487960e-19]), unit=elementary charge**2)

    def __add__(self, other):
        """Add two Quantities.
    
        Only Quantities with the same dimensions (e.g. length)
        can be added.  Raises TypeError otherwise.
    
        Parameters
         - self: left hand member of sum
         - other: right hand member of sum
    
        Returns a new Quantity that is the sum of the two arguments.
        """
        # can only add using like units
        if not self.unit.is_compatible(other.unit):
>           raise TypeError('Cannot add two quantities with incompatible units "%s" and "%s".' % (self.unit, other.unit))
E           TypeError: Cannot add two quantities with incompatible units "elementary charge" and "elementary charge**2".

Which comes from this bit:

        # Modify partial charges so that charge on residue atoms is integral
        # TODO: This may require some modification to correctly handle API changes
        #       when OpenFF toolkit makes charge quantities consistently unit-bearing
        #       or pure numbers.
        _logger.debug(f'Fixing partial charges...')
        _logger.debug(f'{molecule.partial_charges}')
        residue_charge = 0.0 * unit.elementary_charge
        total_charge = molecule.partial_charges.sum()
        sum_of_absolute_charge = np.sum(np.abs(molecule.partial_charges))
        charge_deficit = net_charge - total_charge
        if (sum_of_absolute_charge / unit.elementary_charge).m > 0.0:
            # Redistribute excess charge proportionally to absolute charge
>           molecule.partial_charges = molecule.partial_charges + charge_deficit * abs(molecule.partial_charges) / sum_of_absolute_charge

I think the big tl;dr is that the code is a bit brittle in how it does checking for units, so switching to openff.units is breaking the type checking for the openmm.unit stuff, so then the code does the wrong thing to try and add units to quantities that are unit bearing.

[mattwthompson]

I expected this to be an issue and I was somewhat confused as to why it wasn't while I was working on #227. You're right in that its selection of unit registries is brittle. I assumed (mistakenly) that the conda environment pulled down the old version and the "toolkit False" builds included the old toolkit - and thereby wasn't an issue because everything was passing. But it was just pulling down the new version.

This isn't the only package that's going to run into this tension, and earlier this week I thought of a possible shortcut for downstream developers doing all of the heavy lifting themselves. Could I get some feedback on this PR? Normally I wouldn't ask for something out of scope but I think that change could make this fix easier.

The idea is that we can assume only one version of the toolkit is used in a module, so once we know if that's a newer or older version, you know it'll always return and expect the same registry. Something like

# At the top of the file
from openff.toolkit import __version__ as toolkit_version
from openff.units import ensure_quantity

# This logic should probably replace https://github.com/openmm/openmmforcefields/blob/35496e5095101c9db5ee8a10c7237d1c52dc7f6e/openmmforcefields/generators/template_generators.py#L247
if Version(toolkit_version) >= Version("0.11.0"):
    _TOOLKIT_UNITS = "openff"
else:
    _TOOLKIT_UNITS = "openmm"

# later on ...
        molecule.conformers[0][0,0] += ensure_quantity(0.1*unit.angstroms, _TOOLKIT_UNITS)

This solution should be scalable to any number of calls later on; since the "which version am I using" check happened once, it doesn't need to happen again. Throwing around a bunch of ensure_quantity should make it easier to interact with calls to the toolkit, either sending it quantities or taking quantities from it.

I'd be happy to take this on myself - but I would like a green light if this approach sounds good.

[mattwthompson]

And I guess a bit of added context - I can cut release of openff-units very rapidly, so if we go this route the delay should not be long.

[mikemhenry]

I assumed (mistakenly) that the conda environment pulled down the old version and the "toolkit False" builds included the old toolkit

💯 my fault 🙃 on that!

Let me think about it a bit, I do think we want to make it easy as possible, so I think this proposal is good!

[mattwthompson]

No rush but if you have some thoughts please do comment directly on that PR - ensuring that the units package isn't a one-person show helps safeguard against me putting mistakes out into builds

[mikemhenry]

I'd be happy to take this on myself - but I would like a green light if this approach sounds good.

@mattwthompson consider this a green light ✔️

[mattwthompson]

I am "approving" this in its current state as tests are passing (though some checks are cancelled based on concurrency, I promise they're actually passing https://github.com/openmm/openmmforcefields/actions/runs/3084042229) and it's set out to do everything I think we wanted it. AFAICT

• Version 0.10.6 is actually being pulled down
• Everything seems to work with both versions of the toolkit
• Some tests that I hard-coded to only run with "new" toolkit now work with both versions
• Version-checking cruft has been kept to a minimum

[mattwthompson]

@mikemhenry one tests pass I'd like a review from you (even if brief) and I think this is good to go!

[jchodera]

Pinging @mikemhenry to review!

[mikemhenry]

@mattwthompson I tried to approve it but I opened the PR 🤣

This looks great! I will make sure things work in perses (and some other package that uses this) to make sure we didn't break anything, then get a release on conda-forge!

[mattwthompson]

Great! Please let me know how I can help at any step