Repository: glotzerlab/signac
Pull request #278 (2020-01-27 20:35:25)

Fix project data interface and add unittests for job and project data interfaces
<!-- Provide a general summary of your changes in the Title above -->

## Description
`project.data` used to return a new `H5StoreManager` each time it was called. This prevented users from reading arrays previously stored using the `project.data` interface.

Unit tests covered scalars and strings, but did not cover arrays or pandas dataframes.

## Motivation and Context
Resolves issue #274. Arrays stored using `project.data` cannot be accessed using the `project.data` interface.
These interfaces were not tested comprehensively in unit tests. 
This pull request is still in progress.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.


Example for a changelog entry: `Fix issue with launching rockets to the moon (#101, #212).`


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/278?src=pr&el=h1) Report
> Merging [#278](https://codecov.io/gh/glotzerlab/signac/pull/278?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/e6a278641b10cd9d118a9fab2428f24cfea8d5ef?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@          Coverage Diff          @@
##           master   #278   +/-   ##
=====================================
  Coverage      65%    65%           
=====================================
  Files          40     40           
  Lines        5630   5630           
=====================================
  Hits         3660   3660           
  Misses       1970   1970
```



------

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

klywang: In the `master` branch. The following code properly returns arrays:
``` bash
$ signac shell
>>> pr.data['a'] = np.zeros([10])
>>> with pr.data as store:
...     store.a[:]
... 
array([0., 0., 0., 0., 0., 0., 0., 0., 0., 0.])
```
While 
``` bash
$ signac shell
>>> pr.data['a'] = np.zeros([10])
>>> with pr.data:
...     pr.data.a[:]
... 
```
returns an error. The current unit tests, which inherit from `test_h5store`, pass on the `master` branch since they test an equivelant of the first example.

If the goal is to add unit tests which can catch the bug reported in #274, would it be best to test both instances? 
bdice: @klywang We may choose to merge #275 first, which replaces unittest with pytest. I think it would be easier to update this PR to use pytest.
bdice: @klywang Yes, we should definitely test both cases! Both are valid.
klywang: @bdice Okay I can add tests for both instances after the testing framework has been updated.
bdice: @jennyfothergill Good question! The test classes like `ProjectStoreTest` are actually getting all of their test logic from the parent classes. This is a case of [diamond inheritance](https://en.wikipedia.org/wiki/Multiple_inheritance#The_diamond_problem). I talked with @klywang about this in person, so I can re-cap it here. I apologize in advance for the fact that all the class names sound alike, which makes this hard to read.

The `H5StoreManager` itself was not the cause of the bug -- our _implementation_ of that store manager in the `Project` class had a bug (but this bug wasn't in the `Job` class, for some reason). To prevent this from happening again, we want to test `project.stores` and `project.data` directly (also need to test `job.stores` and `job.data` in a similar way before this PR is finalized). That is, we want tests that do everything that the `H5StoreManager` tests do, except with `project.stores` (which is itself an `H5StoreManager`). We solve that by creating a new class:

```python
class ProjectStoreTest(BaseProjectStoreTest, test_h5store.H5StoreTest):
    pass
```

**Everything in this class (the setup and the tests) is defined by inheritance.** It's not actually empty, as it appears. The `BaseProjectStoreTest` implements the core logic of the `BaseH5StoreTest` (e.g. a `setUp` function to create the project and methods for acquiring the data store, by returning `project.data`). The `BaseH5StoreTest` contains logic shared by all `H5Store` related tests. However, it doesn't actually have any tests. In `test_h5store.py`, you'll see that the tests are defined in `H5StoreTest`. However, we want to override the `BaseH5StoreTest` to use our temporary project's `project.data` instead of an `H5StoreManager` created in a temporary folder. So, we create `BaseProjectStoreTest` and re-define those methods. Then in the `ProjectStoreTest` class, the setup logic comes from `BaseProjectStoreTest` and the actual tests come from `test_h5store.H5StoreTest`.

In picture form:
![image](https://user-images.githubusercontent.com/3943761/73225707-97919d80-413b-11ea-90c0-bd003bab34f1.png)

FYI, this picture (and the class names in particular) may change when we shift to pytest.
jennyfothergill: Oh! Wow, thank you so much for the great explanation! I think I understand.
bdice: @klywang I updated this PR to match pytest. The tests appear to be running correctly with pytest. Next steps:
- [ ] pull the latest version of this branch
- [ ] add the second test instance you were mentioning above to `test_h5store.py` so it will be used by both sets of tests
- [ ] implement the same kind of inherited test classes for the `test_job.py`

If you want to run the tests locally, you'll need to install `pytest` and `pytest-subtests`. Then you can run `pytest test_project.py`, etc.

Use `pytest test_project.py -k "TestProjectStore" -v` to run all the store-related tests.
klywang: While adding tests, I came across these questions:
1. Why is `H5StoreAlreadyOpen` ignored in `H5StoreManager`? This appears to allow users to embed context managers with different modes in their code, but in practice, only opens the file with the mode specifiecd in the initial context manager.
2. How similar should `H5Store` and `H5StoreManager` be? Tests for more advanced usages of `H5Store` fail when the same operations are performed indirectly through the project interface with `H5StoreManager`.

**Context for question 1 & 2**
As of now, the behavior of H5Stores and H5StoreManager behavior differs for several tests. Examples include tests for multiple instances of a opened file in read/write mode:
```python
with project.data.open(mode='w') as h5p:
    with project.data.open(mode='r') as h5p2:
       pass
```
Will return an `H5StoreAlreadyOpen` error while:
```python
with H5Store(filename, mode='w') as h5p:
    with H5Store(filename, mode='r') as h5p:
        pass
```
Will not return an error.

In `h5store.py`, the following lines are included in the `H5Store` context manager:
https://github.com/glotzerlab/signac/blob/569b8e5957ea07f00528ae3cfd1392f1c10d1043/signac/core/h5store.py#L317-L322

If I understand correctly, the context manager should ignore the arguments of the second context manager. To test this, I swapped line 118 and 119  in following test and ran it for `H5Store` rather than the project interface (since that one will fail regardless of order):
https://github.com/glotzerlab/signac/blob/569b8e5957ea07f00528ae3cfd1392f1c10d1043/tests/test_h5store.py#L117-L127

so that the order looked like this:
```python
with self.open_h5store(mode='r') as h5s_r:
    with self.open_h5store(mode='w') as h5s_w:
                assert 'foo' not in h5s_r
                assert 'foo' not in h5s_w

                h5s_w['foo'] = 'bar'
                assert 'foo' in h5s_r
                self.assertEqual(h5s_r['foo'], 'bar')
                assert 'foo' in h5s_w
                self.assertEqual(h5s_r['foo'], 'bar')
```
This test failed at `h5s_w['foo']  = 'bar'` because the file did not exist. This is likely due ot the fact that the `H5Store` was opened in "read" mode and "write" mode was never triggered.
Why are we allowing this sort of behavior and testing it in H5Store? I could attempt to replicate it in the project data interface (and `H5StoreManager`) but it seems like it may be better to allow the `H5StoreAlreadyOpen` error to occur, rather than passing it.

**Context for question 2**
Additionally, 4/5 of the tests in `TestH5StoreMultiProcessing` fail when tested on `project.data` but pass for `H5Store`. These tests seem to test if `__enter__` works correctly. It seems like calling the `H5StoreManager` class in a context manager does not trigger the same behavior as directly calling the `H5Store` class in a context manager. For example, this test assumes that calling the context manager in "append" mode (the default mode set for the context manager in `H5Store`) will create a file:
https://github.com/glotzerlab/signac/blob/569b8e5957ea07f00528ae3cfd1392f1c10d1043/tests/test_h5store.py#L685-L699

But the same test, where `open_h5store` is the yields the context manager for the project data or stores interface does not create a file. I would guess the difference in behavior is the same reason `H5Store` passes the `H5StoreAlreadyOpen` error but the `H5StoreManager` error.
klywang: This should be for review now.

Patch:
diff --git a/changelog.txt b/changelog.txt
index ff6e2f92..19c1c2c7 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -28,6 +28,7 @@ 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).
+ - Fixed issue with ``project.data`` access from separate instances of ``H5StoreManager` (#274, #278).
 
 Removed
 +++++++
diff --git a/contributors.yaml b/contributors.yaml
index 36499f71..8e1f0881 100644
--- a/contributors.yaml
+++ b/contributors.yaml
@@ -83,5 +83,5 @@ contributors:
   -
     family-names: Chandra
     given-names: Abhavya
-    affiliation: "India Institute of Technology, Gandhinagar"    
+    affiliation: "India Institute of Technology, Gandhinagar"
 ...
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 7ff20b4f..e0ed1028 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -53,6 +53,7 @@ class Job(object):
     "The job's document filename."
 
     KEY_DATA = 'signac_data'
+    "The job's datastore key."
 
     def __init__(self, project, statepoint, _id=None):
         self._project = project
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index 66afddb0..2e19da04 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -156,6 +156,9 @@ class Project(object):
         self._fn_doc = os.path.join(self._rd, self.FN_DOCUMENT)
         self._document = None
 
+        # Prepare project h5-stores
+        self._stores = H5StoreManager(self._rd)
+
         # Prepare Workspace Directory
         if not os.path.isdir(self._wd):
             try:
@@ -378,7 +381,7 @@ class Project(object):
         :return: The HDF5-Store manager for this project.
         :rtype: :class:`~..core.h5store.H5StoreManager`
         """
-        return H5StoreManager(self._rd)
+        return self._stores
 
     @property
     def data(self):


Test patch:
diff --git a/tests/test_h5store.py b/tests/test_h5store.py
index ab17587c..d9103807 100644
--- a/tests/test_h5store.py
+++ b/tests/test_h5store.py
@@ -61,7 +61,6 @@ class TestH5StoreBase():
         request.addfinalizer(self._tmp_dir.cleanup)
         self._fn_store = os.path.join(self._tmp_dir.name, FN_STORE)
         self._fn_store_other = os.path.join(self._tmp_dir.name, 'other_' + FN_STORE)
-        
 
     def get_h5store(self, **kwargs):
         return H5Store(filename=self._fn_store, **kwargs)
@@ -87,7 +86,7 @@ class TestH5StoreBase():
     def assertEqual(self, a, b):
         if hasattr(a, 'shape'):
             if not NUMPY:
-                raise unittest.SkipTest("This test requires the numpy package.")
+                raise pytest.skip("This test requires the numpy package.")
             numpy.testing.assert_array_equal(a, b)
         else:
             assert a == b
diff --git a/tests/test_project.py b/tests/test_project.py
index 55c162c3..7da7073a 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -11,12 +11,14 @@ import itertools
 import json
 import pickle
 import string
+import warnings
 import pytest
 from tarfile import TarFile
 from zipfile import ZipFile
 from tempfile import TemporaryDirectory
 from packaging import version
-from contextlib import redirect_stderr
+from contextlib import redirect_stderr, contextmanager
+from time import time
 
 
 import signac
@@ -31,6 +33,7 @@ from signac.contrib.project import JobsCursor, Project  # noqa: F401
 from signac.common.config import get_config
 
 from test_job import TestJobBase
+import test_h5store
 
 
 try:
@@ -46,6 +49,11 @@ try:
 except ImportError:
     H5PY = False
 
+try:
+    import numpy    # noqa
+    NUMPY = True
+except ImportError:
+    NUMPY = False
 
 # Skip linked view tests on Windows
 WINDOWS = (sys.platform == 'win32')
@@ -155,6 +163,7 @@ class TestProject(TestProjectBase):
         assert self.project.doc == {'a': {'b': 45}}
 
     @pytest.mark.skipif(not H5PY, reason='test requires the h5py package')
+    @pytest.mark.skipIf(not NUMPY, reason='test requires the numpy package')
     def test_data(self):
         with self.project.data:
             assert not self.project.data
@@ -178,6 +187,8 @@ class TestProject(TestProjectBase):
             assert self.project.data == {'a': {'b': 43}}
             self.project.data.a.b = 44
             assert self.project.data == {'a': {'b': 44}}
+            self.project.data['c'] = numpy.zeros(10)
+            numpy.testing.assert_array_equal(self.project.data['c'], numpy.zeros(10))
         # This setter will overwrite the file. We leave the context manager so
         # that the file is closed before overwriting it.
         self.project.data = {'a': {'b': 45}}
@@ -2103,3 +2114,136 @@ class TestTestingProjectInitialization(TestProjectBase):
                 assert len(tmp_project) == len(jobs)
                 # check that call does not fail:
                 tmp_project.detect_schema()
+
+
+class TestProjectStoreBase(test_h5store.TestH5StoreBase):
+
+    project_class = signac.Project
+
+    @pytest.fixture(autouse=True)
+    def setUp_base_h5Store(self, request):
+        self._tmp_dir = TemporaryDirectory(prefix='signac_')
+        request.addfinalizer(self._tmp_dir.cleanup)
+        self._tmp_pr = os.path.join(self._tmp_dir.name, 'pr')
+        self._tmp_wd = os.path.join(self._tmp_dir.name, 'wd')
+        os.mkdir(self._tmp_pr)
+        self.config = signac.common.config.load_config()
+        self.project = self.project_class.init_project(
+            name='testing_test_project',
+            root=self._tmp_pr,
+            workspace=self._tmp_wd)
+
+        warnings.filterwarnings('ignore', category=DeprecationWarning, module='signac')
+        self._fn_store = os.path.join(self._tmp_dir.name, 'signac_data.h5')
+        self._fn_store_other = os.path.join(self._tmp_dir.name, 'other.h5')
+
+    def get_h5store(self):
+        return self.project.data
+
+    @contextmanager
+    def open_h5store(self, **kwargs):
+        with self.get_h5store().open(**kwargs) as h5s:
+            yield h5s
+
+    def get_other_h5store(self):
+        return self.project.stores['other']
+
+    @contextmanager
+    def open_other_h5store(self, **kwargs):
+        with self.get_other_h5store().open(**kwargs) as h5s:
+            yield h5s
+
+
+class TestProjectStore(TestProjectStoreBase, test_h5store.TestH5Store):
+
+    """
+    This test opens multiple instances of H5Store, but
+    the project data interface opens one instance of H5Store.
+    This test will (and should) fail using the project data interface.
+    """
+    def test_assign_valid_types_within_same_file(self):
+        pass
+
+
+class TestProjectStoreOpen(TestProjectStoreBase, test_h5store.TestH5StoreOpen):
+
+    """
+    This test opens multiple instances of H5Store, but
+    the project data interface opens one instance of H5Store.
+    This test will (and should) fail using the project data interface.
+    """
+    def test_open_write_and_read_only(self):
+        pass
+
+
+class TestProjectStoreNestedData(TestProjectStore, test_h5store.TestH5StoreNestedData):
+    pass
+
+
+class TestProjectStoreBytes(TestProjectStore, test_h5store.TestH5StoreBytesData):
+    pass
+
+
+class TestProjectStoreClosed(TestProjectStore, test_h5store.TestH5StoreClosed):
+    pass
+
+
+class TestProjectStoreNestedDataClosed(TestProjectStoreNestedData, test_h5store.TestH5StoreNestedDataClosed):
+    pass
+
+
+class TestProjectStorePandasData(TestProjectStore, test_h5store.TestH5StorePandasData):
+    pass
+
+class TestProjectStoreNestedPandasData(TestProjectStorePandasData, test_h5store.TestH5StoreNestedPandasData):
+    pass
+
+
+class TestProjectStoreMultiThreading(TestProjectStoreBase, test_h5store.TestH5StoreMultiThreading):
+    pass
+
+
+class TestProjectStoreMultiProcessing(TestProjectStoreBase, test_h5store.TestH5StoreMultiProcessing):
+
+    """
+    These tests open multiple instances of H5Store, but
+    the project data interface opens one instance of H5Store.
+    Theses tests will (and should) fail using the project data interface.
+    """
+    @contextmanager
+    def open_h5store(self, **kwargs):
+        with signac.H5Store(self.project.fn('signac_data.h5')) as h5:
+            yield h5
+
+    def test_single_writer_multiple_reader_same_instance(self):
+        pass
+
+    def test_multiple_reader_different_process_no_swmr(self):
+        pass
+
+    def test_single_writer_multiple_reader_different_process_no_swmr(self):
+        pass
+
+    def test_single_writer_multiple_reader_different_process_swmr(self):
+        pass
+
+
+class TestProjectStorePerformance(TestProjectStoreBase, test_h5store.TestH5StorePerformance):
+
+    @pytest.fixture
+    def setUp(self, setUp_base_h5Store):
+        value = TestProjectStorePerformance.get_testdata(self)
+        times = numpy.zeros(200)
+        for i in range(len(times)):
+            start = time()
+            with h5py.File(self._fn_store, mode='a') as h5file:
+                if i:
+                    del h5file['_basegroup']
+                h5file.create_group('_basegroup').create_dataset(
+                    '_baseline', data=value, shape=None)
+            times[i] = time() - start
+        self.baseline_time = times
+
+
+class TestProjectStorePerformanceNestedData(TestProjectStorePerformance, test_h5store.TestH5StorePerformance):
+    pass