Repository: glotzerlab/signac

Issue:
Fix race condition when workspace is created
### Description

In MPI parallel jobs, where every rank (or every `n` ranks) may initialize a statepoint on the fly, a race condition leads to the job crashing when no `workspace` directory is present. The explanation is that this directory is created on the fly simultaneously by all `job.init()`s of the same ensemble.

A detailed look identifies the `mkdir -p` in https://github.com/glotzerlab/signac/blob/2c0d9e4710bf9a758c53e74a9ad117acc20d43ae/signac/contrib/job.py#L337
as a culprit. The `mkdir` should not include the `-p` option, instead operate only on the target directory. Job workspaces should be created by default upon `signac init`.

Optionally, when they are deleted (for whatever reason), they could be recreated by an explicit `init` call, which would be properly guarded by MPI barriers. For now, the workaround is to create the workspace dir manually.

### To reproduce

```python
sp = {'just': 'a statepoint'}

project = signac.get_project()

# When initializing jobs on the fly in MPI setting:
job = project.open_job(sp)
device.comm.barrier_all() # HOOMD next
if device.comm.partition == 0 and device.comm.rank == 0:
    job.init() # this is the problematic line
device.comm.barrier_all()
```

This would be the suggested way to do it (pending changes to signac):
```python
sp = {'just': 'a statepoint'}

project = signac.get_project()
device.comm.barrier_all()
if device.comm.partition == 0 and device.comm.rank == 0:
    project.init()
device.comm.barrier_all()

# When initializing jobs on the fly in MPI setting:
job = project.open_job(sp)
device.comm.barrier_all()
if device.comm.partition == 0 and device.comm.rank == 0:
    job.init() # this is the problematic line
device.comm.barrier_all()
```
### Error output

If possible, copy any terminal outputs or attach screenshots that provide additional information on the problem.

```
Error occured while trying to create workspace directory for job '42e1468f85f2b689659f23bae00de995'.
State point manifest file of job '42e1468f85f2b689659f23bae00de995' appears to be corrupted.
Traceback (most recent call last):
  File "1brf_umbrella.py", line 125, in <module>
    job.init()
  File "/ccs/home/glaser/.conda/envs/myenv/lib/python3.7/site-packages/signac/contrib/job.py", line 427, in init
    self._init(force=force)
  File "/ccs/home/glaser/.conda/envs/myenv/lib/python3.7/site-packages/signac/contrib/job.py", line 354, in _init
    _mkdir_p(self._wd)
  File "/ccs/home/glaser/.conda/envs/myenv/lib/python3.7/site-packages/signac/contrib/utility.py", line 171, in _mkdir_p
    os.makedirs(path)
  File "/ccs/home/glaser/.conda/envs/myenv/lib/python3.7/os.py", line 221, in makedirs
    mkdir(name, mode)
...
```
### System configuration

Please complete the following information:

 - Operating System [e.g. macOS]: Linux login2 4.14.0-115.8.1.el7a.ppc64le #1 SMP Thu May 9 14:45:13 UTC 2019 ppc64le ppc64le ppc64le GNU/Linux (really, any OS)
 - Version of Python [e.g. 3.7]: Python 3.7.4 (really, any python3 version)
 - Version of signac [e.g. 1.0]: signac 1.0.0 (but looking at the code the problem may also be present in newer versions)


Discussion:
vishav1771: @jglaser @csadorf Some tests are failing. Can you help me fix the issues?
bdice: @vishav1771 Those tests are checking the assumptions of the old behavior. Since you changed the behavior, you will also need to update those tests. If you look at the test outputs, it will help you find the code to change. See the very bottom of this page for the error outputs: https://circleci.com/gh/glotzerlab/signac/4716

```python
======================================================================
ERROR: test_workspace_read_only_path (test_project.CachedProjectTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/circleci/repo/tests/test_project.py", line 237, in test_workspace_read_only_path
    with open(os.path.join(self.project.workspace()), 'w'):
IsADirectoryError: [Errno 21] Is a directory: '/tmp/signac_5zyno0sl/wd'

======================================================================
ERROR: test_workspace_read_only_path (test_project.ProjectTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/circleci/repo/tests/test_project.py", line 237, in test_workspace_read_only_path
    with open(os.path.join(self.project.workspace()), 'w'):
IsADirectoryError: [Errno 21] Is a directory: '/tmp/signac_qq_kwy8m/wd'

======================================================================
FAIL: test_no_workspace_warn_on_find (test_project.CachedProjectTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/circleci/repo/tests/test_project.py", line 217, in test_no_workspace_warn_on_find
    self.assertFalse(os.path.exists(self.project.workspace()))
AssertionError: True is not false

======================================================================
FAIL: test_no_workspace_warn_on_find (test_project.ProjectTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/circleci/repo/tests/test_project.py", line 217, in test_no_workspace_warn_on_find
    self.assertFalse(os.path.exists(self.project.workspace()))
AssertionError: True is not false
```

For the first two tests, you'll see a comment right above line 237 in `test_project.py`. It says:
```python
        # Create file where workspace would be, thus preventing the creation
        # of the workspace directory.
```
The purpose of `test_workspace_read_only_path` is to check the behavior if a workspace cannot be created. This test makes a **file** called `workspace`, which prevents the creation of the **directory** `workspace`. To fix this test, you'll need to add a line at the beginning of that test that removes the existing workspace directory before it makes the file.

The purpose of `test_no_workspace_warn_on_find` is to make sure that the user is warned if they attempt to search for jobs and no workspace directory is found. You should do the same thing here, just remove the workspace directory at the beginning of the test.
csadorf: > @vishav1771 Those tests are checking the assumptions of the old behavior. Since you changed the behavior, you will also need to update those tests.

Just because it was not explicitly mentioned, I would like to point out that **changing existing unit tests is generally not appropriate** since this means that the patch introduces a change in behavior that is likely backwards-incompatible.

We should therefore carefully analyze in the review 
- what the exact nature of this change is,
- whether it is avoidable, and
- what possible implications it may have on users who implicitly or explicitly rely on the current behavior, and
- whether we should delay the release of this breaking change until the next major version increase.

@vishav1771 All of your changes are fine, I just want to make sure the reviewers are sensitive to this issue.
vishav1771: @bdice I have done the changes you asked.
bdice: @vishav1771 Thanks! I reviewed the PR checklist at the top and there are a couple of next steps:
- Add yourself to the list of contributors (edit `contributors.yaml`)
- Add a **new** test that checks if the workspace directory exists after the project is initialized, so that we guarantee this behavior in the future (you might call it `test_workspace_directory_exists` in `test_project.py`)
- Update the changelog with a short sentence documenting the change in behavior, and include the issue and PR numbers in parentheses (just follow the pattern). Put it under these headers:
```
Version 1
=========

next
----

Added
+++++
(... some stuff ...)

Changed
+++++++
- Your line goes here
```
codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/271?src=pr&el=h1) Report
> Merging [#271](https://codecov.io/gh/glotzerlab/signac/pull/271?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/6bf33e097e6545fe7e1d14fddd6e2f35cd23a4bd?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `60%`.

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

```diff
@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   65.01%   65.01%   -0.01%     
==========================================
  Files          40       40              
  Lines        5626     5631       +5     
==========================================
+ Hits         3658     3661       +3     
- Misses       1968     1970       +2
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/271?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/271/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `90.64% <60%> (-0.23%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/271?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/271?src=pr&el=footer). Last update [6bf33e0...06bb43a](https://codecov.io/gh/glotzerlab/signac/pull/271?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/changelog.txt b/changelog.txt
index d099ad1a..ac6d84a9 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -14,11 +14,19 @@ Added
 +++++
 
  - Added Windows to platforms tested with continuous integration (#264, #266).
+ 
+Changed
++++++++
+
+ - Workspace directory is created when ``Project`` is initialized (#267, #271).
+
 
 Fixed
 +++++
 
  - Fixed issues on Windows with ``H5Store``, project import/export, and operations that move files (#264, #266).
+ - Calling ``items`` or ``values`` on _SyncedDict objects does not mutate nested dictionaries (#234, #269).
+
 
 
 [1.3.0] -- 2019-12-20
diff --git a/contributors.yaml b/contributors.yaml
index 0838169e..150fa054 100644
--- a/contributors.yaml
+++ b/contributors.yaml
@@ -76,4 +76,8 @@ contributors:
     given-names: Brandon
     affiliation: "University of Michigan"
     orcid: "https://orcid.org/0000-0001-7739-7796"
+  -
+    family-names: Sharma
+    given-names: Vishav
+    affiliation: "National Institute of Technology, Hamirpur"
 ...
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index 6864be88..208f23a9 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -156,6 +156,16 @@ class Project(object):
         self._fn_doc = os.path.join(self._rd, self.FN_DOCUMENT)
         self._document = None
 
+        # Prepare Workspace Directory
+        if not os.path.isdir(self._wd):
+            try:
+                _mkdir_p(self._wd)
+            except OSError:
+                logger.error(
+                    "Error occurred while trying to create "
+                    "workspace directory for project {}.".format(self.id))
+                raise
+
         # Internal caches
         self._index_cache = dict()
         self._sp_cache = dict()
diff --git a/signac/core/synceddict.py b/signac/core/synceddict.py
index e3eb1393..48c74555 100644
--- a/signac/core/synceddict.py
+++ b/signac/core/synceddict.py
@@ -265,11 +265,11 @@ class _SyncedDict(MutableMapping):
 
     def values(self):
         self._synced_load()
-        return self._convert_to_dict(self._data).values()
+        return self._convert_to_dict(self).values()
 
     def items(self):
         self._synced_load()
-        return self._convert_to_dict(self._data).items()
+        return self._convert_to_dict(self).items()
 
     def __repr__(self):
         return repr(self())
@@ -279,7 +279,7 @@ class _SyncedDict(MutableMapping):
 
     def _as_dict(self):
         with self._suspend_sync():
-            return self._convert_to_dict(self._data.copy())
+            return self._convert_to_dict(self)
 
     def __call__(self):
         self._synced_load()


Test patch:
diff --git a/tests/test_project.py b/tests/test_project.py
index 32147f77..f3680f40 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -98,6 +98,9 @@ class ProjectTest(BaseProjectTest):
         self.project.config['workspace_dir'] = '${SIGNAC_ENV_DIR_TEST}'
         self.assertEqual(self._tmp_wd, self.project.workspace())
 
+    def test_workspace_directory_exists(self):
+        self.assertTrue(os.path.exists(self.project.workspace()))
+
     def test_fn(self):
         self.assertEqual(
             self.project.fn('test/abc'),
@@ -214,7 +217,8 @@ class ProjectTest(BaseProjectTest):
             norm_path(os.path.join(self.project.root_directory(), self.project.workspace())))
 
     def test_no_workspace_warn_on_find(self):
-        self.assertFalse(os.path.exists(self.project.workspace()))
+        if os.path.exists(self.project.workspace()):
+            os.rmdir(self.project.workspace())
         with self.assertLogs(level='INFO') as cm:
             list(self.project.find_jobs())
             # Python < 3.8 will return 2 messages.
@@ -234,6 +238,8 @@ class ProjectTest(BaseProjectTest):
     def test_workspace_read_only_path(self):
         # Create file where workspace would be, thus preventing the creation
         # of the workspace directory.
+        if os.path.exists(self.project.workspace()):
+            os.rmdir(self.project.workspace())
         with open(os.path.join(self.project.workspace()), 'w'):
             pass
 
diff --git a/tests/test_synced_attrdict.py b/tests/test_synced_attrdict.py
index 503c758d..6509e363 100644
--- a/tests/test_synced_attrdict.py
+++ b/tests/test_synced_attrdict.py
@@ -508,6 +508,18 @@ class SyncedAttrDictTest(unittest.TestCase):
         self.assertEqual(len(sad), 1)
         self.assert_only_read()
 
+    def test_nested_types_dict_conversion(self):
+        """Ensure that calling methods like items and values does not
+        change the type of nested dictionaries."""
+        sad = self.get_sad({'a': {'b': 1}})
+        assert type(sad['a']) is SAD
+        sad.items()
+        assert type(sad['a']) is SAD
+        sad.values()
+        assert type(sad['a']) is SAD
+        sad._as_dict()
+        assert type(sad['a']) is SAD
+
 
 if __name__ == '__main__':
     unittest.main()