Repository: glotzerlab/signac
Pull request #207 (2019-07-14 19:35:38)

Fix/deep copy issue 153 based on issue 168
Implement well-defined copy and deepcopy behavior for instances of `Job`.

This patch is based on the code base of #205 .

Fixes issue #153 .

codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=h1) Report
> Merging [#207](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=desc) into [fix/issue-168](https://codecov.io/gh/glotzerlab/signac/commit/814ad2d7cd31cddfd80fbfe31735ed5ecc64bf30?src=pr&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/207/graphs/tree.svg?width=650&token=FghbjTyzgM&height=150&src=pr)](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=tree)

```diff
@@                Coverage Diff                @@
##           fix/issue-168     #207      +/-   ##
=================================================
+ Coverage          64.99%   65.09%   +0.09%     
=================================================
  Files                 37       37              
  Lines               5577     5589      +12     
=================================================
+ Hits                3625     3638      +13     
+ Misses              1952     1951       -1
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/207/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `90.53% <100%> (+0.49%)` | :arrow_up: |
| [signac/core/dict\_manager.py](https://codecov.io/gh/glotzerlab/signac/pull/207/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvZGljdF9tYW5hZ2VyLnB5) | `90.69% <0%> (+1.16%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=footer). Last update [814ad2d...5b4e778](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=h1) Report
> Merging [#207](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=desc) into [fix/issue-168](https://codecov.io/gh/glotzerlab/signac/commit/814ad2d7cd31cddfd80fbfe31735ed5ecc64bf30?src=pr&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/207/graphs/tree.svg?width=650&token=FghbjTyzgM&height=150&src=pr)](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=tree)

```diff
@@                Coverage Diff                @@
##           fix/issue-168     #207      +/-   ##
=================================================
+ Coverage          64.99%   65.09%   +0.09%     
=================================================
  Files                 37       37              
  Lines               5577     5589      +12     
=================================================
+ Hits                3625     3638      +13     
+ Misses              1952     1951       -1
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/job.py](https://codecov.io/gh/glotzerlab/signac/pull/207/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvam9iLnB5) | `90.53% <100%> (+0.49%)` | :arrow_up: |
| [signac/core/dict\_manager.py](https://codecov.io/gh/glotzerlab/signac/pull/207/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvZGljdF9tYW5hZ2VyLnB5) | `90.69% <0%> (+1.16%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=footer). Last update [814ad2d...5b4e778](https://codecov.io/gh/glotzerlab/signac/pull/207?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

csadorf: Note: Merge after #205 .
csadorf: @vyasr This patch has the unit tests from #154 , but in addition has unit tests for the copy/deepcopy behavior of instances of `Job` and a patch of the `Job` class that fixes the `copy` behavior. I suggest we merge this PR instead of #154 .
csadorf: @vyasr I've answered your questions.
csadorf: > I like the current unittests and implementation.
> 
> From my understanding to make sure I have it correct is that copy effectively links changes to all copies while deepcopy does not propagate changes in statepoint to other objects that have references to it. However, all copies of either type point to the same underlying file unless a deepcopied object or the original job object is changed at which point only the changed one still points to an underlying file.
> 
> This is the behavior that is implied by the tests.

That's exactly correct.

Patch:
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index fe632166..001f95e0 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -5,6 +5,7 @@ import os
 import errno
 import logging
 import shutil
+from copy import deepcopy
 
 from ..common import six
 from ..core import json
@@ -22,14 +23,15 @@ logger = logging.getLogger(__name__)
 
 class _sp_save_hook(object):
 
-    def __init__(self, job):
-        self.job = job
+    def __init__(self, *jobs):
+        self.jobs = list(jobs)
 
     def load(self):
         pass
 
     def save(self):
-        self.job._reset_sp()
+        for job in self.jobs:
+            job._reset_sp()
 
 
 class Job(object):
@@ -144,7 +146,7 @@ class Job(object):
             else:
                 raise
         # Update this instance
-        self._statepoint = SyncedAttrDict(dst._statepoint._as_dict(), parent=_sp_save_hook(self))
+        self._statepoint._data = dst._statepoint._data
         self._id = dst._id
         self._wd = dst._wd
         self._fn_doc = dst._fn_doc
@@ -579,3 +581,15 @@ class Job(object):
     def __exit__(self, err_type, err_value, tb):
         self.close()
         return False
+
+    def __setstate__(self, state):
+        self.__dict__.update(state)
+        self._statepoint._parent.jobs.append(self)
+
+    def __deepcopy__(self, memo):
+        cls = self.__class__
+        result = cls.__new__(cls)
+        memo[id(self)] = result
+        for k, v in self.__dict__.items():
+            setattr(result, k, deepcopy(v, memo))
+        return result


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index 6a8e0eb6..5bc1e1b5 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -153,6 +153,80 @@ class JobTest(BaseJobTest):
             file.write('hello')
         self.assertTrue(job.isfile(fn))
 
+    def test_copy(self):
+        job = self.project.open_job({'a': 0}).init()
+        self.assertIn(job, self.project)
+
+        # Modify copy
+        copied_job = copy.copy(job)
+        self.assertFalse(job is copied_job)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        copied_job.sp.a = 1
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+
+        # Modify original
+        copied_job = copy.copy(job)
+        self.assertFalse(job is copied_job)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        job.sp.a = 2
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+
+        # Delete original
+        del job
+        self.assertIn(copied_job, self.project)
+        copied_job.sp.a = 3
+        self.assertIn(copied_job, self.project)
+
+    def test_deepcopy(self):
+        job = self.project.open_job({'a': 0}).init()
+        self.assertIn(job, self.project)
+
+        # Modify copy
+        copied_job = copy.deepcopy(job)
+        self.assertFalse(job is copied_job)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        copied_job.sp.a = 1
+        self.assertNotIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        self.assertNotEqual(job, copied_job)
+        self.assertNotEqual(job.sp, copied_job.sp)
+
+        # Modify original
+        job = self.project.open_job({'a': 0}).init()
+        copied_job = copy.deepcopy(job)
+        self.assertFalse(job is copied_job)
+        self.assertEqual(job, copied_job)
+        self.assertEqual(job.sp, copied_job.sp)
+        self.assertIn(job, self.project)
+        self.assertIn(copied_job, self.project)
+        job.sp.a = 2
+        self.assertIn(job, self.project)
+        self.assertNotIn(copied_job, self.project)
+        self.assertNotEqual(job, copied_job)
+        self.assertNotEqual(job.sp, copied_job.sp)
+
+        # Delete original
+        copied_job = copy.deepcopy(job)
+        del job
+        self.assertIn(copied_job, self.project)
+        copied_job.sp.a = 3
+        self.assertIn(copied_job, self.project)
+
 
 class JobSPInterfaceTest(BaseJobTest):
 
@@ -252,6 +326,16 @@ class JobSPInterfaceTest(BaseJobTest):
         self.assertNotIn('a', job.sp)
         self.assertEqual(job.sp.b, 0)
 
+    def test_interface_copy(self):
+        job = self.open_job(dict(a=0)).init()
+        copy.copy(job.sp).a = 1
+        self.assertTrue(job in self.project)
+
+    def test_interface_deepcopy(self):
+        job = self.open_job(dict(a=0)).init()
+        copy.deepcopy(job.sp).a = 1
+        self.assertFalse(job in self.project)
+
     def test_interface_add(self):
         job = self.open_job(dict(a=0))
         job.init()
@@ -1447,6 +1531,26 @@ class JobOpenDataTest(BaseJobTest):
             self.assertIn(key, dst2_job.data)
             self.assertEqual(len(dst2_job.data), 1)
 
+    def test_statepoint_copy(self):
+        job = self.open_job(dict(a=test_token, b=test_token)).init()
+        _id = job.get_id()
+        sp_copy = copy.copy(job.sp)
+        del sp_copy['b']
+        self.assertIn('a', job.sp)
+        self.assertNotIn('b', job.sp)
+        self.assertIn(job, self.project)
+        self.assertNotEqual(job.get_id(), _id)
+
+    def test_statepoint_deepcopy(self):
+        job = self.open_job(dict(a=test_token, b=test_token)).init()
+        _id = job.get_id()
+        sp_copy = copy.deepcopy(job.sp)
+        del sp_copy['b']
+        self.assertIn('a', job.sp)
+        self.assertIn('b', job.sp)
+        self.assertNotIn(job, self.project)
+        self.assertEqual(job.get_id(), _id)
+
 
 @unittest.skipIf(not H5PY, 'test requires the h5py package')
 class JobClosedDataTest(JobOpenDataTest):