plumpy.ProcessListener made persistent #274
Conversation
rikigigi
force-pushed
the
persistent_listener
branch
from
9b738f8
to
4fd678b
Compare
|
@sphuber I don't understand why 4 test fail here https://github.com/aiidateam/plumpy/actions/runs/6012847906/job/16309013751?pr=274 ... do you have any idea? Thank you very much |
|
The |
|
I think that the reason that prevented call_with_super_check from working was the fact that it used a single counter for all function. In my case it was called multiple times in the stack. Unfortunately to make the persistence of the Listener, I had to introduce some ugly hacks in the test suite to avoid circular references and to correctly compare snapshots. |
|
The timeout issue is not present in my local run of the test suite :-( |
|
did a simple test with aiidateam/aiida-core#5732 rebased on current develop + https://github.com/sphuber/aiida-s3 and it worked fine, with the ProcessListener being correctly called on the aiida daemon |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #274 +/- ##
==========================================
- Coverage 90.83% 90.76% -0.06%
==========================================
Files 21 22 +1
Lines 2974 2995 +21
==========================================
+ Hits 2701 2718 +17
- Misses 273 277 +4
☔ View full report in Codecov by Sentry. |
|
@sphuber just a ping. Are the changes in the call_with_super_check ok? Thank you |
|
Sorry for the delay @rikigigi . Could you please give a small writeup as to why these hacks were necessary and what they are doing? |
|
I reverted the changes on the call_with_super_check, but a small details that prints the name of the correct function in case of error. They were not necessary. The issue with the tests is simple: the |
| @@ -376,6 +424,11 @@ def compare_value(bundle1, bundle2, v1, v2, exclude=None): | |||
| elif isinstance(v1, list) and isinstance(v2, list): | |||
| for vv1, vv2 in zip(v1, v2): | |||
| compare_value(bundle1, bundle2, vv1, vv2, exclude) | |||
| elif isinstance(v1, set) and isinstance(v2, set) and len(v1) == len(v2) and len(v1) <= 1: | |||
There was a problem hiding this comment.
Am I going nuts or are the bundle1 and bundle2 arguments in compare_dictionaries and compare_value not doing anything whatsoever? If true and they do not do anything, do we really need this custom dictionary compare function? Surely there must be existing tools that provide this? I realize this is not something you added in this PR, but just wondering if I am missing something
There was a problem hiding this comment.
I think you are right, bundle1 and bundle2 are useless! https://github.com/seperman/deepdiff may be an option, but I don't think I will add it in this PR
|
Also, please install Finally, maybe you want to merge this into https://github.com/aiidateam/plumpy/tree/support/0.21.x instead. |
|
ok, thank you! I'm working on it. |
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
force-pushed
the
persistent_listener
branch
from
ca0644e
to
a830eef
Compare
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
force-pushed
the
persistent_listener
branch
from
a830eef
to
8724b83
Compare
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
|
I addressed the issues and created a different PR for 0.21.x support #277 |
|
I am closing this as I cherry-picked your fix that was released with the v0.21.10 support in #279 so it is now also on |
solves #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