Serialize FileIO and TextIOWrapper and Universe by yuxuanzhuang · Pull Request #2723 · MDAnalysis/mdanalysis

yuxuanzhuang · 2020-06-09T16:23:15Z

Fixes #2878

Composition over inheritance for pickling Universe in Updating Serialization Functionality from PR #2140 #2704

Changes made in this Pull Request:

add new picklable BufferedReader, FileIO, and TextIOWrapper classes.
add new classes and pickle_open function to picklable_file_io.py
implement __getstate__, __setstate__ to Universe and BaseReader
fix DCD, XDR pickle issue

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

update to HEAD

codecov · 2020-06-09T17:59:01Z

Codecov Report

Merging #2723 into develop will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2723      +/-   ##
===========================================
+ Coverage    92.80%   92.86%   +0.06%     
===========================================
  Files          185      186       +1     
  Lines        24205    24383     +178     
  Branches      3133     3149      +16     
===========================================
+ Hits         22463    22643     +180     
+ Misses        1696     1694       -2     
  Partials        46       46

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/DLPoly.py	`98.80% <100.00%> (+<0.01%)`	⬆️
package/MDAnalysis/coordinates/GSD.py	`91.66% <100.00%> (+2.77%)`	⬆️
package/MDAnalysis/coordinates/TRJ.py	`97.69% <100.00%> (+0.02%)`	⬆️
package/MDAnalysis/coordinates/base.py	`94.88% <100.00%> (+0.06%)`	⬆️
package/MDAnalysis/coordinates/chain.py	`92.60% <100.00%> (+0.33%)`	⬆️
package/MDAnalysis/coordinates/chemfiles.py	`94.88% <100.00%> (+0.40%)`	⬆️
package/MDAnalysis/core/universe.py	`97.19% <100.00%> (+0.24%)`	⬆️
package/MDAnalysis/lib/__init__.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/lib/formats/libdcd.pyx	`89.33% <100.00%> (+0.03%)`	⬆️
package/MDAnalysis/lib/formats/libmdaxdr.pyx	`89.96% <100.00%> (+0.03%)`	⬆️
... and 8 more

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 2550d06...5ace1e0. Read the comment docs.

richardjgowers · 2020-06-09T20:47:26Z

@yuxuanzhuang ignore the failing Python 2.7 tests, once we release 1.0 we're not supporting Python 2 any more.

orbeckst

general comments

orbeckst · 2020-06-10T17:46:46Z

package/MDAnalysis/lib/util.py

+        return buffer
+    elif mode == 'rt' or mode == 'r':
+        return TextIOPickable(buffer)
+


I think this should fail for anything else (ValueError for unsupported mode).

Maybe add a comment here saying that we should never get here; it's not obvious until you read all code up to this point.

Or better

assert False, "mode = {} argument was never processed".format(mode)

Do you mean replacing the ValueError for unsupported mode?

orbeckst · 2020-06-10T17:46:52Z

package/MDAnalysis/lib/util.py

+
+# not as comprehensive as built-in open func--no need for other args
+# only should be used for 'reading' modes
+def pickle_open(name, mode):


package/MDAnalysis/lib/util.py

orbeckst · 2020-06-10T17:49:01Z

testsuite/MDAnalysisTests/utils/test_pickleio.py

+def test_offset(f):
+    f.readline()
+    f_pickled = pickle.loads(pickle.dumps(f))
+    assert_equal(f.tell(),f_pickled.tell())


PEP8 - space after comma

I can recommend running a formatting tool with every save. I use black for python nowadays.

IAlibay

just a couple of formatting/test changes

package/MDAnalysis/lib/util.py

testsuite/MDAnalysisTests/utils/test_pickleio.py

package/MDAnalysis/lib/util.py

yuxuanzhuang · 2020-06-11T13:27:24Z

I guess these func and classes should be something for internal use, i.e. being called by anyopen, instead of being called directly?

orbeckst · 2020-06-11T17:54:02Z

I guess these func and classes should be something for internal use, i.e. being called by anyopen, instead of being called directly?

This might be the case right now. But sometimes they find other uses, too. You could start out with them being "private" (but still fully documented) and we can later make them public with a defined API.

lib.util is getting really big and unwieldy. Perhaps it makes sense to split off a fileutil module (or something with a better name, io is used everywhere but is nice and short)? – Opinions?

orbeckst

Overall this looks very good and promising. I have a bunch of comments and suggestions (see inline), mainly

docs
suggestion to internally change the order of the pickle (purely for aesthetics and code readability)
spelling: should be "picklable" instead of "pickable" everywhere; I made suggestions but didn't correct everything (such as all the tests are still "pickable")

If pickle_open() is used elsewhere then we should also provide a corresponding context manager. I don't know how urgent that is and it does not need to happen in this PR.

As I also said in another comment, it might be worthwhile splitting lib.utils and moving all file/IO related functionality into its own module.

package/MDAnalysis/lib/util.py

orbeckst · 2020-06-11T18:56:37Z

package/MDAnalysis/lib/util.py

+        return buffer
+    elif mode == 'rt' or mode == 'r':
+        return TextIOPickable(buffer)
+


Maybe add a comment here saying that we should never get here; it's not obvious until you read all code up to this point.

Or better

assert False, "mode = {} argument was never processed".format(mode)

testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py

richardjgowers · 2020-06-12T07:32:20Z

Yeah I'd put this in a new file then maybe import it into the .lib namespace. So it's available as MDAnalysis.lib.PicklableFileIO

orbeckst · 2020-06-12T07:40:16Z

@richardjgowers do you mean a module picklable_file_io.py (modules are traditionally lower case in Python and in MDA)? Maybe call it pio.py for "picklable I/O"?

richardjgowers · 2020-06-12T07:45:55Z

@orbeckst the file can be called anything, so either of those are fine. I don't think a user needs to understand the file layout of things, instead in lib/__init__.py:

from picklable_file_io import FileIOPicklable, ...

so lib.FileIOPicklable is the class not the module

jbarnoud · 2020-08-03T10:14:44Z

testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py

+    ('CHAIN', [PDB, PDB, PDB], dict()),
+    ('CHAIN', [XTC, XTC, XTC], dict()),
+])
+def ref_reader(request):


Would it make sense to put that test in BaseReaderTest (and maybe _SingleFrameReader) so any new reader get tested? While it looks cleaner to have the test here, I am worried we will miss readers, especially when new readers get added.

Do you mean cover all the _SingleFrameReader as how class TestPDBReader(_SingleFrameReader) is implemented? As for pickle test in BaseReaderTest and MutliframeReaderTest, they were already added.

It is what I meant, indeed. My concern when I see this fixture is that we will forget to add the new readers to the list. Ideally I would like something that fails if a reader is not tested for pickling and multiprocessing.

I think for now, only tests in NAMDBIN, PDB, PQR utilize _SingleFrameReader. Makes sense to create a new PR for other single frame readers e.g. MMTF, to be inherited from that? (which it along can be backported.)

I'd be in favor of getting the PR merged and open a new one where we think hard about how to do consistent testing of all our readers and parsers.

We'll also need to look at converters... but that's a different issue. Literally.

testsuite/MDAnalysisTests/coordinates/base.py

IAlibay

I think any further questions I have re: documentation can/should be documented in the userguide or similar. I'm good with this, thanks for all the work @yuxuanzhuang !

orbeckst · 2020-08-07T18:42:38Z

ping reviewers @IAlibay @fiona-naughton – can you please check if your comments were addressed? If yes, please approve the PR. If not, please point out specifically what needs to be done, in the form of actionable requests. As discussed on the call, we'd like to get this merged ASAP so that @yuxuanzhuang can move forward.

orbeckst · 2020-08-07T18:43:03Z

Oops... sorry @IAlibay – you approved while I was writing my message. Thanks!

fiona-naughton · 2020-08-07T23:58:07Z

looks good to me - nice work @yuxuanzhuang !

orbeckst · 2020-08-08T00:41:13Z

@yuxuanzhuang congratulations – it is done! 🎉

yuxuanzhuang · 2020-08-08T09:49:13Z

Thanks for all the help!

- Fixes #2878 - Changes made in this Pull Request: - add more comprehensive tests for pickling low-level dcd and xdr files - fix a bug that was introduced in #2723 (wrong seeking in DCD and XDR, note: this bug was only in develop, never in released code, see discussion in PR #2908) - update CHANGELOG - maybe backport in #2908

* Fixes MDAnalysis#2878 * basic approach: composition instead of inheritance for pickling Universe (which was tested in PR MDAnalysis#2704 (which was derived from PR MDAnalysis#2140)) * Changes made in this Pull Request: - add new classes and pickle_open function to picklable_file_io.py - add new picklable `BufferedReader`, `FileIO`, and `TextIOWrapper` classes. - implement `__getstate__`, `__setstate__` to `Universe` and `BaseReader` - fix DCD, XDR pickle issue - modified gsd and ncdf to be picklabel - modified ChainReader to be picklabel - ensure chemfiles is picklable * add tests (MultiFrameReader will test for serializability) * update CHANGELOG * update docs Note: This merge squashed 120 commits. See PR MDAnalysis#2723 for the full history with 420 comments.

- Fixes MDAnalysis#2878 - Changes made in this Pull Request: - add more comprehensive tests for pickling low-level dcd and xdr files - fix a bug that was introduced in MDAnalysis#2723 (wrong seeking in DCD and XDR, note: this bug was only in develop, never in released code, see discussion in PR MDAnalysis#2908) - update CHANGELOG - maybe backport in MDAnalysis#2908

yuxuanzhuang added 5 commits May 20, 2020 10:32

Merge pull request #1 from MDAnalysis/develop

931d9b5

update to HEAD

Merge remote-tracking branch 'mda_origin/develop' into develop

d69ce98

add pickle function to fileio, textio

432edf3

add basic test for pickle io

bf55bf3

add comments

cd9a485

xfail on python2

f33629f

orbeckst reviewed Jun 10, 2020

View reviewed changes

yuxuanzhuang added 5 commits June 11, 2020 12:52

add doc and exception for pickle_open

aca4496

add doc for textio fileio class

cd4ffe3

add parallel test for textio

ec5bd3c

pep8

f6515ee

add an extra bufferlayer for FileIO for fast access

cf29764

IAlibay requested changes Jun 11, 2020

View reviewed changes

yuxuanzhuang added 5 commits June 11, 2020 14:23

Merge remote-tracking branch 'mda_origin/develop' into serialize_io

6b09e20

ditch py2

c29c316

pep8 and doc

db47e27

add test for unsupported mode

7e9d6d3

pep8

eb83a7c

orbeckst requested changes Jun 11, 2020

View reviewed changes

yuxuanzhuang added 2 commits June 12, 2020 09:24

typo

0baa868

pickle reorder

c8e63b2

yuxuanzhuang added 2 commits June 12, 2020 10:26

pickle_open as context manager

658b446

format

1aa6003

jbarnoud reviewed Aug 3, 2020

View reviewed changes

yuxuanzhuang added 2 commits August 3, 2020 16:32

add pickle test to single_framereader

c99867f

add pickle test to fhiams

a70bc8b

orbeckst reviewed Aug 4, 2020

View reviewed changes

testsuite/MDAnalysisTests/coordinates/base.py Show resolved Hide resolved

yuxuanzhuang added 3 commits August 6, 2020 17:18

test doc

bc487a5

test doc title

a1bb47e

test doc title 2

5ace1e0

IAlibay approved these changes Aug 7, 2020

View reviewed changes

orbeckst added the GSoC GSoC project label Aug 7, 2020

fiona-naughton approved these changes Aug 7, 2020

View reviewed changes

orbeckst merged commit 563995c into MDAnalysis:develop Aug 8, 2020

orbeckst mentioned this pull request Aug 10, 2020

replace deprecated imp with importlib #2897

Closed

yuxuanzhuang mentioned this pull request Aug 15, 2020

DCD XDR pickle tests #2911

Merged

4 tasks

This was referenced Aug 16, 2020

Issue 1940 pickle chain reader #1945

Closed

Parallel trajectory analysis (revisited) #618

Closed

fiona-naughton added Component-Core parallelization labels Sep 26, 2023

Conversation

yuxuanzhuang commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

codecov bot commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richardjgowers commented Jun 9, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuxuanzhuang commented Jun 11, 2020

Uh oh!

orbeckst commented Jun 11, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

richardjgowers commented Jun 12, 2020

Uh oh!

orbeckst commented Jun 12, 2020

Uh oh!

richardjgowers commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Aug 7, 2020

Uh oh!

orbeckst commented Aug 7, 2020

Uh oh!

fiona-naughton commented Aug 7, 2020

Uh oh!

yuxuanzhuang commented Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 9, 2020 •

edited

Loading