Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProcessListener added to Process are not persistent #273

Closed
rikigigi opened this issue Aug 28, 2023 · 2 comments
Closed

ProcessListener added to Process are not persistent #273

rikigigi opened this issue Aug 28, 2023 · 2 comments

Comments

@rikigigi
Copy link
Member

I would expect that after reloading a checkpoint, the listener that I added to the Process is persisted. This can be useful in the AiiDA daemon. For example, I may add a listener that connects to a remote URL to notify when the WorkChain has finished. I apologize if I have misunderstood the intended functionality.

Below you can find an example of code that does not load the listener after a checkpoint is loaded.

Thank you.

import plumpy

persister = plumpy.InMemoryPersister()

class TestListener(plumpy.ProcessListener):

    def on_process_excepted(self, process, reason):
        print('process excepted')
    
    def on_process_killed(self, process, reason):
        print('process killed')

    def on_process_finished(self, process, output):
        print('process finished')

class SimpleWorkChain(plumpy.WorkChain):
    @classmethod
    def define(cls, spec):
        super().define(spec)
        spec.outline(
            cls.step1,
            cls.step2,
        )

    def step1(self):
        print('step1')
        persister.save_checkpoint(self,'step1')

    def step2(self):
        print('step2')
        persister.save_checkpoint(self,'step2')
        
workchain = SimpleWorkChain()
workchain.add_process_listener(TestListener())
output = workchain.execute()

print('reload persister checkpoint:')
workchain_checkpoint = persister.load_checkpoint(workchain.pid, 'step1').unbundle()
workchain_checkpoint.execute()
@sphuber
Copy link
Collaborator

sphuber commented Aug 28, 2023

I had a look through the source code and it seems that the event listeners are simply never persisted, and so they are also not reconstructed when the process is reloaded from a checkpoint. This functionality is not actually used in aiida-core so that's why it has gone unnoticed. Would be happy to accept a PR. You essentially need to update the Process.save_instance_state and Process.load_instance_state to serialize and deserialize the listeners into the Savable.

rikigigi added a commit to rikigigi/plumpy that referenced this issue Aug 29, 2023
solves aiidateam#273

We implement the persistence of ProcessListener by deriving the class
ProcessListener and EventHelper from persistence.Savable.
The class EventHelper is moved to a new file because of a circular
import with utils and persistence
rikigigi added a commit to rikigigi/plumpy that referenced this issue Aug 29, 2023
solves aiidateam#273

We implement the persistence of ProcessListener by deriving the class
ProcessListener and EventHelper from persistence.Savable.
The class EventHelper is moved to a new file because of a circular
import with utils and persistence
rikigigi added a commit to rikigigi/plumpy that referenced this issue Nov 10, 2023
solves aiidateam#273

We implement the persistence of ProcessListener by deriving the class
ProcessListener and EventHelper from persistence.Savable.
The class EventHelper is moved to a new file because of a circular
import with utils and persistence

Fixing the test

There was a circular reference issue in the test listener that was
storing a reference to the process inside it, making its serialization
impossible. To fix the tests an ugly hack was used: storing the
reference to the process outside the class in a global dict using id as
keys. Some more ugly hacks are needed to check correctly the equality of
two processes. We must ignore the fact that the instances if the
listener are different.

We call del on dict items of the ProcessListener's global implemented in the test suite
to clean the golbal variables

addressed issues in aiidateam#274
rikigigi added a commit to rikigigi/plumpy that referenced this issue Nov 10, 2023
solves aiidateam#273

We implement the persistence of ProcessListener by deriving the class
ProcessListener and EventHelper from persistence.Savable.
The class EventHelper is moved to a new file because of a circular
import with utils and persistence

Fixing the test

There was a circular reference issue in the test listener that was
storing a reference to the process inside it, making its serialization
impossible. To fix the tests an ugly hack was used: storing the
reference to the process outside the class in a global dict using id as
keys. Some more ugly hacks are needed to check correctly the equality of
two processes. We must ignore the fact that the instances if the
listener are different.

We call del on dict items of the ProcessListener's global implemented in the test suite
to clean the golbal variables

addressed issues in aiidateam#274
rikigigi added a commit to rikigigi/plumpy that referenced this issue Nov 10, 2023
solves aiidateam#273

We implement the persistence of ProcessListener by deriving the class
ProcessListener and EventHelper from persistence.Savable.
The class EventHelper is moved to a new file because of a circular
import with utils and persistence

Fixing the test

There was a circular reference issue in the test listener that was
storing a reference to the process inside it, making its serialization
impossible. To fix the tests an ugly hack was used: storing the
reference to the process outside the class in a global dict using id as
keys. Some more ugly hacks are needed to check correctly the equality of
two processes. We must ignore the fact that the instances if the
listener are different.

We call del on dict items of the ProcessListener's global implemented in the test suite
to clean the golbal variables

addressed issues in aiidateam#274
@rikigigi
Copy link
Member Author

fixed by #277 and #279

and tested in the combination

plumpy @ git+https://github.com/aiidateam/plumpy@01dc91dc7767ff56a76c4d62cddeda832243b69d
aiida-core @ git+https://github.com/aiidateam/aiida-core@8b8887e559e02eadac832a89f7012872040e1cbc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants