Add MutableLocalFileStorage subclass skeleton#749
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
This seems on the right track, but yeah it's quite barebones. I certainly don't have objections or major suggestions for anything here since the bulk of the work comes in the follow-ups
* add copy not move * add equilibration tests * rm extraneous tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* add ability to combine storages * add copy not move * add equilibration tests * rm extraneous tests * add ability to combine storages * fix merge conflicts... * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix merge ????!!! * why am i so bad at this * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix tests * turn on ci * Apply suggestions from code review Co-authored-by: Matt Thompson <matt.thompson@openforcefield.org> * update assert --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Matt Thompson <matt.thompson@openforcefield.org>
* Add subset code and tests * update tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * update * add type hints --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Add remove obj * make tests compatible * fix class name
|
Ok -- I think that's each of the sub-PRs merged in! @mattwthompson did you want to re-review this, or is it good to go for |
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
I'm pleasantly surprised at the size of the final changeset - I think our approach of merging smaller changes into a staging trunk was correct (and I would do it the same if I could go back in time) but my concern that the final change would be illegibly large has not come true
I want to see what silicon has to say about it - but only for my curiosity, I don't think its feedback should block this being merged in
There was a problem hiding this comment.
Pull request overview
Introduces a new MutableLocalFileStorage backend to enable “copy-on-store” behavior and provide basic mutating/compositional operations for local storages, supporting the storage-manipulation workflows requested in #748.
Changes:
- Added
MutableLocalFileStoragewithupdate/+=,subset(...), andremove_object(...), and copy-on-store semantics for ancillary directories. - Exported
MutableLocalFileStoragefromopenff.evaluator.storage. - Expanded storage test coverage to include
StoredEquilibrationDataalongsideStoredSimulationData, and added a dedicated test module for the mutable backend.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openff/evaluator/storage/mutable.py | New mutable LocalFileStorage subclass with copy-on-store, merge/subset, and deletion APIs |
| openff/evaluator/storage/init.py | Re-export MutableLocalFileStorage |
| openff/evaluator/_tests/utils.py | Add helper to create dummy StoredEquilibrationData test objects |
| openff/evaluator/_tests/test_storage/test_storage.py | Parametrize existing storage tests over simulation vs equilibration data/query types |
| openff/evaluator/_tests/test_storage/test_mutable.py | New tests for copy-on-store, update/+= merge, subset filters, and remove_object behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Related to #748 . This will function as a trunk PR -- I'll make additional PRs into this branch for easier review, then merge this if/when everything gets an approval.
As such no tests at the moment, but would be good to still get a quick review on this for anything that pops out.