Drop Pydantic v1#93
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
+ Coverage 97.66% 99.51% +1.85%
==========================================
Files 11 11
Lines 1071 1038 -33
==========================================
- Hits 1046 1033 -13
+ Misses 25 5 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Documentation build overview
37 files changed ·
|
Keys have historically matched on (id, mult, associated_handler, bond_order) only. After dropping the pydantic v1 shim, matching uses PotentialKey's full-field equality, which also compares virtual_site_type and cosmetic_attributes -- fields a user cannot reconstruct from a SMIRKS. Such keys are then silently ignored in exclude (and would raise KeyError in include). Covers both vulnerable fields via the exclude path: virtual_site_type (a v-site key) and cosmetic_attributes (any handler; here a cosmetic attribute on a Bonds parameter). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
I didn't find any problems myself, but I let claude do a once-over and it spotted a sneaky regression wrt vsites and cosmetic attributes. I worked with claude to generate minimal tests expressing this behavior.
I lean slightly toward favoring fixing this regression (since it could cause user surprise) but I'd also be fine with A) just listing it in the release notes (and removing the new tests I committed) or B) erroring in cases where it could make a difference (though this could be hard to detect)
| excluded = openff.interchange.models.PotentialKey( | ||
| id="[#1:2]-[#8X2H2+0:1]-[#1:3] EP once", | ||
| associated_handler="VirtualSites", | ||
| ) |
There was a problem hiding this comment.
This is now how as-applied virtual site parameters are stored in Interchange. There is a separate class for virtual sites that intentionally stores relevant fields:
In [1]: from openff.toolkit import Molecule, ForceField
In [2]: virtual_site_keys = [*ForceField("opc.offxml").create_interchange(Molecule.from_smiles("O").
⋮ to_topology())['VirtualSites'].key_map]
In [3]: virtual_site_keys
Out[3]: [SMIRNOFFVirtualSiteKey with orientation atom indices (0, 1, 2)]
In [4]: type(virtual_site_keys)
Out[4]: list
In [5]: [type(key) for key in virtual_site_keys]
Out[5]: [openff.interchange.models.SMIRNOFFVirtualSiteKey]
In [6]: virtual_site_keys[-1].dict()
<ipython-input-6-9d453a18a1da>:1: PydanticDeprecatedSince20: The `dict` method is deprecated; use `model_dump` instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.13/migration/
virtual_site_keys[-1].dict()
Out[6]:
{'atom_indices': None,
'orientation_atom_indices': (0, 1, 2),
'type': 'DivalentLonePair',
'name': 'EP',
'match': 'once'}
This approach, be it by Claude or our scientists, of smooshing together 3 parameters into a single string is not encouraged
| all_keys = [ | ||
| openff.interchange.models.PotentialKey(**key.dict()) | ||
| for key in force_field.v_sites.keys | ||
| ] |
There was a problem hiding this comment.
Ideally switch these over to VirtualSiteKeys
There was a problem hiding this comment.
Apologies for wading in here, but I'd like to understand this. My understanding is: we want the keys to match force field parameters independent of which atoms they're applied to, but VirtualSiteKey is a subclass of TopologyKey, so identifies where parameters are matched rather than the parameters themselves. PotentialKeys identify parameters, independent of where they're applied. So how can we replace PotentialKeys with VirtualSiteKeys?
Also, from above, is the "smooshing together" of parameters not a fairly old feature of Interchange? https://github.com/openforcefield/openff-interchange/blob/100e7f09923c8f42a64672dade85a5e9aa256a2c/openff/interchange/smirnoff/_virtual_sites.py#L121
Thanks!
There was a problem hiding this comment.
You're right - I confused myself with these classes and what they do. I'll take a stab at fixing this regression - would you be up for reviewing a fix for this? ETA mid next week
I do think there could be a better solution than stringifying the relevant fields together, but that's what is done in Interchange and ought to be what this code builds off of.
| key_to_row = {key: row_idx for row_idx, key in enumerate(all_keys)} | ||
| assert len(key_to_row) == len(all_keys), "duplicate keys found" | ||
|
|
||
| return {key_to_row[key] for key in unfrozen_keys if key not in excluded_keys} |
There was a problem hiding this comment.
Note to self: this is the point of failure. It's not actually related to Pydantic API changes, it's related to Interchange behavior changes over some period of years. The ~vendored code here has different behavior than upstream.
If we want exclude a fairly generic parameter
ipdb> [e.dict() for e in excluded_keys]
[{'id': '[#6X4:1]-[#6X4:2]', 'mult': None, 'associated_handler': 'Bonds', 'bond_order': None, 'virtual_site_type': None, 'cosmetic_attributes': {}}]but a similar-looking parameter also has cosmetic attributes:
ipdb> [u.dict() for u in unfrozen_keys if u.cosmetic_attributes]
[{'id': '[#6X4:1]-[#6X4:2]', 'mult': None, 'associated_handler': 'Bonds', 'bond_order': None, 'virtual_site_type': None, 'cosmetic_attributes': {'foo': 'bar'}}]then nothing is excluded (probably one parameter should be):
ipdb> len(unfrozen_keys)
2
ipdb> len({key_to_row[key] for key in unfrozen_keys if key not in excluded_keys})
2Optionally add a breakpoint before returning and run something like
$ pixi run -e dev python -m pytest -v descent/tests/test_train.py::TestTrainable::test_init_exclude_key_without_cosmetic_attributes --pdb -s
3 participants
Description
Fixes #91
Status