From 52818ea6665dc8c24284476cc384b088c7367a81 Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 15:27:33 -0400 Subject: [PATCH 1/6] FIX: loadpkl failed when pklz file contained versioning info loadpkl called pickle.load at file position 0, which would fail if the first line of the file was json versioning info (as when using savepkl with versioning=True). This was fixed by only seeking to position 0 if no versioning information is found. --- nipype/tests/test_utils.py | 17 +++++++++++++++++ nipype/utils/filemanip.py | 2 -- 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 nipype/tests/test_utils.py diff --git a/nipype/tests/test_utils.py b/nipype/tests/test_utils.py new file mode 100644 index 0000000000..6a35f6cf85 --- /dev/null +++ b/nipype/tests/test_utils.py @@ -0,0 +1,17 @@ +from nipype import utils + + +def test_pickle(tmp_path): + testobj = 'iamateststr' + pickle_fname = str(tmp_path / 'testpickle.pklz') + utils.filemanip.savepkl(pickle_fname, testobj) + outobj = utils.filemanip.loadpkl(pickle_fname) + assert outobj == testobj + + +def test_pickle_versioning(tmp_path): + testobj = 'iamateststr' + pickle_fname = str(tmp_path / 'testpickle.pklz') + utils.filemanip.savepkl(pickle_fname, testobj, versioning=True) + outobj = utils.filemanip.loadpkl(pickle_fname) + assert outobj == testobj diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 44654c0197..26859196fc 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -689,8 +689,6 @@ def loadpkl(infile, versioning=False): pkl_metadata_line = pkl_file.readline() pkl_metadata = json.loads(pkl_metadata_line) except (UnicodeDecodeError, json.JSONDecodeError): - pass - finally: # Could not get version info pkl_file.seek(0) From c6047defb99987ad0f1fe1a32f34e7d281c4a767 Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 16:31:58 -0400 Subject: [PATCH 2/6] parametrize test (suggested by @effigies) --- nipype/tests/test_utils.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/nipype/tests/test_utils.py b/nipype/tests/test_utils.py index 6a35f6cf85..7c7bc40acd 100644 --- a/nipype/tests/test_utils.py +++ b/nipype/tests/test_utils.py @@ -1,17 +1,11 @@ from nipype import utils +import pytest -def test_pickle(tmp_path): +@pytest.mark.parametrize("versioning", [True, False]) +def test_pickle(tmp_path, versioning): testobj = 'iamateststr' pickle_fname = str(tmp_path / 'testpickle.pklz') - utils.filemanip.savepkl(pickle_fname, testobj) - outobj = utils.filemanip.loadpkl(pickle_fname) - assert outobj == testobj - - -def test_pickle_versioning(tmp_path): - testobj = 'iamateststr' - pickle_fname = str(tmp_path / 'testpickle.pklz') - utils.filemanip.savepkl(pickle_fname, testobj, versioning=True) + utils.filemanip.savepkl(pickle_fname, testobj, versioning=versioning) outobj = utils.filemanip.loadpkl(pickle_fname) assert outobj == testobj From 56ab01ebf50395b4d8d4f112ea3dd1a7459d49f6 Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 16:32:20 -0400 Subject: [PATCH 3/6] remove versioning option from loadpkl --- nipype/utils/filemanip.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 26859196fc..e599f38e4c 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -675,7 +675,7 @@ def loadcrash(infile, *args): raise ValueError('Only pickled crashfiles are supported') -def loadpkl(infile, versioning=False): +def loadpkl(infile): """Load a zipped or plain cPickled file.""" infile = Path(infile) fmlogger.debug('Loading pkl: %s', infile) @@ -700,9 +700,6 @@ def loadpkl(infile, versioning=False): fmlogger.info('Successfully loaded pkl in compatibility mode.') # Unpickling problems except Exception as e: - if not versioning: - raise e - if pkl_metadata and 'version' in pkl_metadata: from nipype import __version__ as version if pkl_metadata['version'] != version: From a86d1409514625ee6bd6ee47c715e33bcf44aaab Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 16:54:54 -0400 Subject: [PATCH 4/6] Revert "remove versioning option from loadpkl" This reverts commit 56ab01ebf50395b4d8d4f112ea3dd1a7459d49f6. --- nipype/utils/filemanip.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index e599f38e4c..26859196fc 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -675,7 +675,7 @@ def loadcrash(infile, *args): raise ValueError('Only pickled crashfiles are supported') -def loadpkl(infile): +def loadpkl(infile, versioning=False): """Load a zipped or plain cPickled file.""" infile = Path(infile) fmlogger.debug('Loading pkl: %s', infile) @@ -700,6 +700,9 @@ def loadpkl(infile): fmlogger.info('Successfully loaded pkl in compatibility mode.') # Unpickling problems except Exception as e: + if not versioning: + raise e + if pkl_metadata and 'version' in pkl_metadata: from nipype import __version__ as version if pkl_metadata['version'] != version: From d3a15e94592d2e157bd9fefd209d889632203b58 Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 17:17:01 -0400 Subject: [PATCH 5/6] test versioning in loadpkl --- nipype/tests/test_utils.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/nipype/tests/test_utils.py b/nipype/tests/test_utils.py index 7c7bc40acd..180ce58172 100644 --- a/nipype/tests/test_utils.py +++ b/nipype/tests/test_utils.py @@ -2,10 +2,11 @@ import pytest -@pytest.mark.parametrize("versioning", [True, False]) -def test_pickle(tmp_path, versioning): +@pytest.mark.parametrize("load_versioning", [True, False]) +@pytest.mark.parametrize("save_versioning", [True, False]) +def test_pickle(tmp_path, save_versioning, load_versioning): testobj = 'iamateststr' pickle_fname = str(tmp_path / 'testpickle.pklz') - utils.filemanip.savepkl(pickle_fname, testobj, versioning=versioning) - outobj = utils.filemanip.loadpkl(pickle_fname) + utils.filemanip.savepkl(pickle_fname, testobj, versioning=save_versioning) + outobj = utils.filemanip.loadpkl(pickle_fname, versioning=load_versioning) assert outobj == testobj From bbc696f344d497a0a41e17e29b55c7448d894bc3 Mon Sep 17 00:00:00 2001 From: Steven Tilley Date: Thu, 5 Sep 2019 17:38:59 -0400 Subject: [PATCH 6/6] move loadpkl tests to proper location --- nipype/tests/test_utils.py | 12 ------------ nipype/utils/tests/test_filemanip.py | 10 ++++++++++ 2 files changed, 10 insertions(+), 12 deletions(-) delete mode 100644 nipype/tests/test_utils.py diff --git a/nipype/tests/test_utils.py b/nipype/tests/test_utils.py deleted file mode 100644 index 180ce58172..0000000000 --- a/nipype/tests/test_utils.py +++ /dev/null @@ -1,12 +0,0 @@ -from nipype import utils -import pytest - - -@pytest.mark.parametrize("load_versioning", [True, False]) -@pytest.mark.parametrize("save_versioning", [True, False]) -def test_pickle(tmp_path, save_versioning, load_versioning): - testobj = 'iamateststr' - pickle_fname = str(tmp_path / 'testpickle.pklz') - utils.filemanip.savepkl(pickle_fname, testobj, versioning=save_versioning) - outobj = utils.filemanip.loadpkl(pickle_fname, versioning=load_versioning) - assert outobj == testobj diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index 7eaa8b9c86..ecfd477504 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -587,3 +587,13 @@ def test_Path_strict_resolve(tmpdir): # If the file is created, it should not raise open('somefile.txt', 'w').close() assert '%s/somefile.txt' % tmpdir == '%s' % testfile.resolve(strict=True) + + +@pytest.mark.parametrize("load_versioning", [True, False]) +@pytest.mark.parametrize("save_versioning", [True, False]) +def test_pickle(tmp_path, save_versioning, load_versioning): + testobj = 'iamateststr' + pickle_fname = str(tmp_path / 'testpickle.pklz') + savepkl(pickle_fname, testobj, versioning=save_versioning) + outobj = loadpkl(pickle_fname, versioning=load_versioning) + assert outobj == testobj