🐛 FIX: Allow for dereferencing of saved instance state

[chrisjsewell]

As a toy example:

import plumpy

persister = plumpy.InMemoryPersister()

class PersistWorkChain(plumpy.WorkChain):

    @classmethod
    def define(cls, spec):
        super().define(spec)
        spec.outline(
            cls.step1,
            cls.step2,
        )
        
    def step1(self):
        self.ctx.step = 1
        persister.save_checkpoint(self, 'step1')

    def step2(self):
        self.ctx.step = 2
        persister.save_checkpoint(self, 'step2')

        
workchain = PersistWorkChain()
workchain.execute()
{k: v['_context'] for k, v in persister._checkpoints[workchain.pid].items()}

{'step1': {'step': 2}, 'step2': {'step': 2}}

This obviously won't affect Persisters that serialize the data, but it is definitely a bug in this instance.

Strictly we should use copy.deepcopy, but this could run into issues if a value was not pickleable, and then I'm not sure how exceptions should be handled

[muhrin]

Good catch. Yes, this is a problem (for InMemoryPersister at least). There are (at least) two strategies one can adopt here:

1. What you've implemented, as you say, ideally with a deepcopy as here we're just kicking the can down one reference along.
2. In a 'proper' programming language one woud have things that enter the bundle be constant references (or pointers to constant data) and the contract with the persister is that it has to serialise or copy the bundle immediately after the save_instance_state call. Naturally, this limits the the assumptions a piece of code can make about a Bundle produced by a save_instance_state call.

Option 1. is almost certainly safer while option 2 is, in general, more performant (and potentially safer if we don't use a deepcopy). Option 2. would require a change to InMemoryPersister (and maybe others if they're relying on having a copy).

Do you have a preference? I'm fairly happy with either so long as there is just a sentence added to the save_instance_method docstring explaining the choice.

[chrisjsewell]

Hmm, yeh I think (2) then, because I'm "scared" that deepcopy has the potential to introduce failures.
save_instance_state should probably be named something like get_instance_state, to not imply it is creating a "static" state, but that's way too much effort lol.

[chrisjsewell]

@muhrin how about a7b8aca?

[codecov-io]

Codecov Report

Merging #191 (a7b8aca) into develop (16e7713) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #191      +/-   ##
===========================================
+ Coverage    90.94%   90.94%   +0.01%     
===========================================
  Files           22       22              
  Lines         2922     2924       +2     
===========================================
+ Hits          2657     2659       +2     
  Misses         265      265              

Impacted Files	Coverage Δ
plumpy/mixins.py	91.31% <ø> (ø)
plumpy/persistence.py	93.94% <100.00%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16e7713...a7b8aca. Read the comment docs.