A first pass at storage

liamhuber commented 10 months ago

@samwaseda, @JNmpi -- saving and loading is working! Needs this branch, and the tinydata branch on contrib.

@pmrv, before I dig deep and start fleshing out tests and examples, I would love to get some high-level feedback from you about the architecture and how I'm (probably mis)using tinybase storage tools.

Design

Uses tinybase.storage.H5ioStorage to save and load nodes (individual function nodes, macros, and workflows). Is written using this branch in contrib, so it is not surprising when tests fail!

Node (and some children) and DataChannel define methods from storing their data in/restoring their data from an H5ioStorage instance, although the DataChannel

The expectation is that loading happens from an already-live instance of a Node; that means that it has already instantiated its child DataChannel objects and they are live too -- so we are really just populating attributes of live objects.

Information about channel connections is stored at the Composite level, and these are recreated after all children are available.

Macros instantiate their children at their own instantiation time, but Workflows need to look at the stored data (package identifier and node class), re-register packages, and re-instantiate their child nodes. Then these nodes can be restored from HDF as usual.

We delay interaction with pyiron_contrib until the last possible moment, as the import still takes forever.

Features

save() and load() interface available at the Node level -- works for function nodes, macros, and workflows
Newly created nodes will try to load themselves by file, if a matching save file exists

It works like this:

from pyiron_workflow import Workflow

Workflow.register("pyiron_workflow.node_library.atomistics", "atomistics")

wf = Workflow("ev_curve", overwrite_save=True)

wf.structure = wf.create.atomistics.task.Bulk("Al")
wf.calculator = wf.create.atomistics.calculator.Emt()

wf.ev = wf.create.atomistics.macro.EnergyVolumeCurve(
    structure=wf.structure, 
    calculator=wf.calculator,
)

out = wf()

wf.save()
out.ev__result_dict["bulkmodul_eq"]
>>> 39.494306059591686

Then we can come back later, in a fresh python interpreter (with the same cwd so we can find the save file), and do this:

from pyiron_workflow import Workflow

wf = Workflow("ev_curve")

wf.outputs.ev__result_dict.value["bulkmodul_eq"]
>>> 39.494306059591686

Note that at load-time we didn't need to register any packages -- everything we needed was found in the stored data and registered automatically.

Shortcomings

~Children of Composite and children of Function need to have macro_creator and node_function defined as class methods, not passed at instantiation, or I'm 99% sure saving/loading will break. This is not too bad, since all the nodes created by the decorators fulfill this automatically, and that's both our preferred and most common way of doing things.~ This is a non-issue; if you define a new Node instance and save it with the same name, indeed, you'll get garbage -- IMO this is just a special case of changing the source code between saving and loading. If you set up the same node instance both times, it saves and loads just fine because it has the necessary function again anyhow.
If you change source code for any nodes between saving and loading, all bets are off
To be saved, all of a Workflow's child nodes need to be created from a package (wf.create....) -- this is so they get a package_identifier and the loading workflow can re-instantiate them. This is not too much of a pain right now since our package tracking is pretty sloppy -- a package is just a module -- so you just need to move nodes from your jupyter notebook over to a .py file somewhere in your python path and register them like usual prior to saving.
There is no filtering of any type right now -- all the IO data gets saved to full depth
If you modify a Macro instance in your notebook (e.g. by changing its internal connections, or replacing an internal node), you'll still be able to save it, but loading it will probably just break
The interaction with the storage uses a lot of by-hand string keys that need to be synced in saving and loading, and what gets saved is all manually included, both of which feel like very brittle implementation to me

TODO:

[x] High-level design review (@pmrv)
- ~Stack a PR: Pull in tinybase from contrib~ Do it later, after contrib 949 is merged
- ~Stack a PR: Rebase classes onto Storable (at least Node -- maybe ChannelData still gets a different solution) so we can use _re/store directly instead of the bespoke to/from_storage~ Do it later, we have an issue placeholder #162
Add more functionality
- [x] Relate storage file to the parent-most node (cf. #165)
- [x] Give direct access to child node storage based on the storage file and a semantic path (cf. #167)
- [x] A node attribute to flag whether a save should be done post-run
[x] TESTS
[x] Docs
[x] examples
~Wait for tinybase tools used to be available on conda~ Git-copy it to this repo instead (and later, per above)

Closes #153

github-actions[bot] commented 10 months ago

:point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/simplestorage

liamhuber commented 10 months ago

I didn't have time today to read in depth, but will do so before our meeting tomorrow.

Super. Indeed, if the academic agenda permits, spending time talking about this stuff live would be both enjoyable and helpful!

One thing that occurs to me right now: to_storage/from_storage is very similar to the Storable interface. I guess implementing restore would be more difficult in your setup because you wanted the objects to be live before populating from storage, but it would make other things much easier (no need to keep track of class names in storage, it already saves a simple version tag). If this is a blocker I would rather discuss what needs to change in Storable to enable it to be used here.

So there are two fundamental objects we're storing: DataChannel and Node. The channels are owned by nodes, and take their parent node as an arg at instantiation time. This sort of reciprocal link is super convenient when everything is alive, and making it a mandatory arg is perfectly sensible under those conditions, but it's hell for storage. So, for DataChannel I think we're stuck defining some sort of bespoke data-extraction routine. Thankfully this is so far quite simple.

For Node I tend to agree with you. I did try sticking Storable into the MRO, but ran into two problems, both of which are probably resolvable:

I just couldn't get it to do what I wanted. I really suspect this is from lack of trying/insufficiently deep understanding of Storable and surrounding classes, and probably not a fundamental problem. We're doing all sorts of nonsense with decorators and dynamically generated classes, but I'm still cautiously optimistic the only real problem was my idiocy.
Importing from contrib is too damned slow, and if Node inherits from it directly then I have to import at boot time and can't delay it until I go out of my way to call .save(). This is resolvable by getting tinybase out on its own somewhere, so is only a short-term barrier. But I don't want to merge and release a solution that requires the "pyiron" boot time, as right now getting running with workflows is pretty snappy.

codacy-production[bot] commented 10 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
:x: -2.76% (target: -1.00%)	:white_check_mark: 35.48%

Coverage variation details

| | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (cc93042ccd4247504d1e2d982e20c49e53d0bd59) | 2591 | 2213 | 85.41% | | | Head commit (eda307ab9b99b2b1ac52954402fd9ef7e0345793) | 2744 (+153) | 2268 (+55) | 82.65% (**-2.76%**) | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `

Diff coverage details

| | Coverable lines | Covered lines | Diff coverage | | ------------- | ------------- | ------------- | ------------- | | Pull request (#160) | 155 | 55 | **35.48%** | **Diff coverage** is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: `/ * 100%`

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

github-actions[bot] commented 10 months ago

Pull Request Test Coverage Report for Build 7589086727

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report. To ensure accuracy in future PRs, please see these guidelines. A quick fix for this PR: rebase it; your next report should be accurate.

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.3%) to 89.002%

Totals
Change from base Build 7576987663:	-1.3%
Covered Lines:	4799
Relevant Lines:	5392

💛 - Coveralls

liamhuber commented 10 months ago

Ok, so we can now get the storage file itself from the parent-most node using #165, and then the path inside the storage to a particular child using #167. That means that children look in the right place in the root-parent's folder and we can do things like:

>>> wf.ev.calc.storage["outputs/energy_dict/strict_hints"]
1

This is nice.

Tangentially, this led me to the question: @pmrv are we still struggling with restoring bools instead of 0/1, or is there just a bug somewhere? Should I still care about this?

liamhuber commented 10 months ago

Also @pmrv: with the failing tests on pyiron_contrib:tinydata and the fairly deep-ish integration (I need to pull out pyiron_base...HasGroups too), I am considering delaying all the TODOs with moving stuff from repo to repo until a later PR. That would mean sticking with the to_storage and from_storage methods for now and introducing tests at the save/load interface level. The big "pro" for me here is to get functioning storage live on main as an alpha feature. Is the palatable to you?

Long run I still am expecting to get Storable in the MRO, I just want to introduce an intermediate state to main where we have storage without that.

pmrv commented 10 months ago

I think the bool issue is related using json for dicts inside h5io, but I would have to check.

Test failure for tinybase is probably contained to the notebooks, so it should be quick on Monday, but if you want thus in main before, go ahead.

review-notebook-app[bot] commented 10 months ago

Check out this pull request on