(Experimental) Pickle: Consider Series identity when unpickling - #1922
(Experimental) Pickle: Consider Series identity when unpickling#1922franzpoeschel wants to merge 18 commits into
Conversation
| # https://github.com/openPMD/openPMD-api/issues/1919 | ||
| # The code is adapted from the reproducer in there. | ||
| try: | ||
| from tqdm.contrib.concurrent import process_map |
There was a problem hiding this comment.
Do you add tqdm as a requirement for your CI? Otherwise, this will never be tested. You could maybe also rewrite it with Pool().map() from multiprocessing if you want to avoid the extra dependency.
There was a problem hiding this comment.
Not yet, I have added it as a TODO
I want to give the CI one try on the test without fix anyway, so it will come up latest at that point
There was a problem hiding this comment.
pytest has @skipif that we can use for missing imports.
There was a problem hiding this comment.
I agree we can probably reproduce this with multiprocessing from the Python stdlib
There was a problem hiding this comment.
@ax3l Pyodide does not seem to have multiprocessing. I don't know if it's just not in our CI run or if it's legitimately not there. The easiest way out would be to just add a try...except block around the import again.
| struct unpickled_series | ||
| { | ||
| std::map<uintptr_t, Series> m_series_by_former_id; | ||
| std::shared_mutex m_mutex; |
There was a problem hiding this comment.
An idea:
Usually when we create series in Python, the Python interpreter owns the lifetime. Now after pickle, there is a C++ object owning the lifetime.
There might be a way to attach the extra data/counter/id to the Python object dynamically and reference it (or the series that is de-serialized first) between each other in Python.
There was a problem hiding this comment.
The last commit tries sth like that
39a53b4 to
9f28d8e
Compare
e1730b3 to
2cfe46a
Compare
…nces Add comprehensive tests for the pickle/unpickle cache mechanism that fixes issue openPMD#1919: - testPickleMultipleSeriesMultipleReferences: Tests the newly introduced cache works correctly with multiple Series objects, each with multiple handles referencing it (iteration, particles, records, components) - testPickleCleanupWithClose: Tests memory cleanup triggered by explicit Series.close() - cleanup happens on next unpickle of a previously not opened Series - testPickleCleanupWithGC: Tests memory cleanup triggered by Python garbage collection - ReadMomentum helper class: Picklable class for multiprocessing tests (required to be a class, not nested function, to be picklable) All tests verify that pickling and unpickling maintains correct references and data consistency across multiple Series instances and multiple handles.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
f576863 to
6e8dee0
Compare
4 participants
This tries fixing the bug reported in #1919 by @pordyna by storing and reading additionally the immutable internal Series memory location. If the location of an unpickled Series is identical to the memory location of a previously unpickled Series, then that Series is reused.
Notes:
The cache of unpickled Series objects is stillthread_localat the moment. This keeps thread safety at the cost of restricting this bugfix to uses in the same thread.Suggestion: When unpickling from the same
Seriesobject, users should expect to share the same internal data references. So it might be fine to use a per-process cache instead of a per-thread cache.The cache is never emptied, so this can be a memory leak.We could maybe add a scan operation that erases closedSeriesobjects from the cache?Series::close()thread_local Seriescache variable from (template) function scope to global scope. This would not fix the entire problem, but it would be a fix for use cases that only use oneSeries.