Handle PluginCompatibilityError in create_openmm_system()

[aehogan]

Currently trying to use a nonbonded plugin from smirnoff-plugins with openff-evaluator throws PluginCompatibilityError from interchange because the default behavior is combine_nonbonded_forces=True. This patch doesn't change the default behavior but trys to set that flag to False if an exception occurs.

• [x] Tag issue being addressed
• [ ] Add tests
• [x] Update docstrings/documentation, if applicable
• [x] Lint codebase
• [ ] Update changelog

[codecov[bot]]

Codecov Report

Merging #1704 (9d61ca9) into main (7fd1a21) will decrease coverage by 11.79%. The diff coverage is 83.33%.

Additional details and impacted files

[mattwthompson]

Nice job looking out for maintaining the default behavior

I think I'd advocate for this change to happen in Evaluator itself, but I also think it's worth considering the long-term maintainability of this block wherever it lands. Right now the only plugin compatibility check can be handled by flipping that argument, since that's the only type of plugin that's implemented, but future plugins aren't guaranteed to follow this pattern. Ideally there's a way to let the user wire in the non-default behavior that their plugin(s) need without affecting the default, long-standing behavior of the toolkit.

Written in code, I'd be worried that this in 5 years turns into

try:
    # current code
except PluginCompatibilityError as error:
    if "combine_nonbonded_forces=False" in str(error):
    # return self.create_interchange(...).to_openmm(combine_nonbonded_forces=False)
    elif "some other failure case" in str(error):
    # handle this one
    ...

etc., etc.

[aehogan]

Right, this code should be in evaluator or interchange imo but this is the smallest change needed to get plugins working in evaluator

[j-wags]

Thanks for taking the initative to suggest a fix, @aehogan! But in addition to @mattwthompson's concerns, I think this would be detrimental to the Toolkit because we'd either:

1) have slow runtimes if a plugin operates normally by triggering this try/except, or 2) put this feature in with the intent to have it only be temporary which would complicate version compatibilities.

I'd be ok with exposing the combine_nonbonded_forces kwarg in create_openmm_system, but that wouldn't fully solve the current problem since you'd still need changes in at least Evaluator (and the problem could be fully solved by changes in Evaluator).

The code in this PR looks like it should work correctly, so I'm assuming you're able to install from this branch and not have your current work blocked.

But ultimately I think this isn't a wanted change in the Toolkit, and that the long-term fix should go into Evaluator/Interchange.

Does this make sense, or am I overlooking anything?

[aehogan]

Yes, this is the smallest temporary fix that lets my forcefields as well as DEXP work with evaluator.

I agree that exposing combine_nonbonded_forces in this function and then changing the way evaluator works would be the best fix, would you like me to close this pull request and then do that instead? It isn't much more work.

[mattwthompson]

I think the best fix would be to just handle this in Evaluator since that would require only one change and only one release. If the change is as small as I expect, it should be quick to write up and add a test for. I'd be happy to quickly review a PR to Evaluator an unblock your work.

It's worth pointing out that for plugins, generally, our preference is for users to use ForceField.create_interchange and then use the Interchange API from there with whatever options make the plugins happy. For this reason I think - as a matter of personal opinion - we should avoid letting Interchange's keyword arguments creep into the toolkit.

[j-wags]

Yeah - I'm with Matt on this one - It'd be best to have Evaluator directly use create_interchange and adjust the use of combine_nonbonded_forces there , with no changes to the Toolkit (since the Toolkit already offers the needed functionality, just at a different API point). So opening a PR on Evaluator would be best!