OpenFreeEnergy / gufe

grand unified free energy by OpenFE
https://gufe.readthedocs.io
MIT License
28 stars 8 forks source link

[WIP] Fix for migration of settings #279

Closed dwhswenson closed 7 months ago

dwhswenson commented 8 months ago

Migration of settings is not working as intended in OpenFE.

Fundamentally, the issue is that we have two serialization mechanisms: one for GufeTokenizables, and one for any other objects. For GufeTokenizables, we have explicit to_dict/from_dict methods. For all other objects, we use the custom JSON. As a matter of practice, objects must be reloaded from JSON before they are inserted into GufeTokenizables, so objects using the custom JSON path are deserialized before GufeTokenizables.

However, this becomes a problem for settings migration, because settings use the custom JSON serialization path. So they get reconstituted before we hit the migration hooks. This leads to errors if, e.g., you rename a setting.

As of the first commit in this PR, this should fix some problems downstream, but there aren't tests in place to ensure that it is at all future proofed. I think the approach here may not be the best, long-term, but this is a start to a band-aid for it.

pep8speaks commented 8 months ago

Hello @dwhswenson! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 110:27: E261 at least two spaces before inline comment Line 110:28: E262 inline comment should start with '# '