Implement the H5StoreManager class. #129

csadorf · 2019-02-19T02:18:48Z

Description

Introduces the DictManager and the H5StoreManager classes that are exposed as job.stores and project.stores.

Motivation and Context

It would be highly advantageous to enable users to easily manage multiple HDF5-files instead of just one. There are plenty of reasons why that is a good idea:

While the HDF5-format is generally mutable, it is fundamentally designed to be used as an immutable data container. In fact, even the latest version is internally restricted to append-only operations and a file will continue to grow with repeated addition and edits. The only way to reduce the file size is to create a new file (e.g. with h5repack).
A single large file is difficult to synchronize. Even if we only change a few MBs or even KBs of a multiple TBs-large file, there is a large chance that synchronization tools like rsync or GLOBUS will have to transfer the complete file. Furthermore, all data stored within one large file share one modification time stamp, making fine-grained synchronization even more difficult. Finally, synchronization of parts of a file are impossible.
A single large file, especially when it is repeatedly mutated poses a data corruption risk. The HDF5 library uses file locks to reduce the risk of data corruption, but there is no journaling as of yet and the corruption of parts of the file, e.g., through a crashed application (think job killed due to walltime stop etc.) might corrupt the whole file leading to complete data loss.
The HDF5-format does not play well with concurrent/parallel access. While the single-writer-multiple-reader (SWMR) mode is implemented for some libraries, it comes with severe limitations and requires a special access style that cannot be easily made transparent to the user. Existence checks etc would be much easier to execute concurrently if they are kept to the file system level without needing to open the file.

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

This pull request is based on

master, because it is a bug fix or an update to the documentation.
develop, because it introduces a new feature.

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
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 on signac-docs and linked it here.
I have updated the changelog.

csadorf · 2019-02-20T21:15:04Z

@glotzerlab/signac-maintainers This is ready for review.

Manages multiple instances of H5Store within a specific directory. Exposed as job.stores and project.stores.

…Store.

Register returned dicts in internal registry to ensure persistence of returned dicts over the lifetime of the manager instance.

In addition to testing 'job.data', we also test 'job.stores.test'.

This patch fixes an issue that required users to explicitly hold a reference to each store returned from `job.stores`, otherwise the returned container would be immediatley deleted prior to use.

mikemhenry

LGTM, I don't work with the H5Store enough to approve the PR but looking over it I don't see anything obviously wrong. We should make sure to update glotzerlab/signac-docs#6 in light of the changes made here.

csadorf · 2019-02-21T19:46:39Z

LGTM, I don't work with the H5Store enough to approve the PR but looking over it I don't see anything obviously wrong. We should make sure to update glotzerlab/signac-docs#6 in light of the changes made here.

Thank you, that's a good point and I will go through the docs before merging.

vyasr · 2019-02-23T02:34:12Z

I will review this tomorrow, trying to finish up another task before I start going back through signac PRs.

vyasr

This is a pretty minimal set of changes to enable the new use-case, I like it. Just a couple small things and we're good to go.

changelog.txt

signac/contrib/project.py

signac/core/dict_manager.py

vyasr · 2019-02-24T02:41:49Z

signac/core/dict_manager.py

+                pass
+            raise error
+        else:
+            del self._dict_registry[key]


I don't follow why this is necessary. The file corresponding to key was successfully created if we reach this point, why does it need to be removed from the tracked list?

It's not really necessary, I figured it might be a good idea to replace any references of "old" H5Store under the same key, but there is virtually no difference. I'll remove that.

Turns out this is actually necessary. I tried to delete this line and the test_assign and test_assign_data unit tests started to fail.

vyasr · 2019-02-24T02:43:40Z

signac/core/dict_manager.py

+        for fn in os.listdir(self.prefix):
+            m = re.match('(.*){}'.format(self.suffix), fn)
+            if m:
+                yield m.groups()[0]


Using re seems like overkill here, I'd switch this to using glob.glob: yield from glob.glob("*.{}".format(self.suffix)) (without using yield from for py2 compatibility).

I can't directly extract the key name with glob.

I suppose you'd still have to do something like ...replace(self.suffix, '') to get just the key without the extension, that's all that you're referring to right? I personally think that's still clearer than using a regex since you're really just parsing out an extension from a set of filenames, but I'm OK with leaving it as is if you prefer since it doesn't simplify the code that much.

I just tried it. I would also need to strip off the directory name, so I'm sticking to the original implementation.

However, should we explicitly skip hidden h5-files?

I don't think that's necessary. I view creating and editing hidden hd5 files as a feature, not a bug. If users explicitly access a hidden file I think it's reasonable to expect that they know what they're doing.

csadorf

@vyasr Thank you for your review. I'll address any required code changes tomorrow.

csadorf · 2019-02-24T04:51:06Z

signac/core/dict_manager.py

+                pass
+            raise error
+        else:
+            del self._dict_registry[key]


It's not really necessary, I figured it might be a good idea to replace any references of "old" H5Store under the same key, but there is virtually no difference. I'll remove that.

csadorf · 2019-02-24T04:52:29Z

signac/core/dict_manager.py

+        for fn in os.listdir(self.prefix):
+            m = re.match('(.*){}'.format(self.suffix), fn)
+            if m:
+                yield m.groups()[0]


I can't directly extract the key name with glob.

bdice · 2019-02-24T18:13:02Z

tests/test_job.py

+            self.assertIn(key, job.stores.test)
+            self.assertEqual(job.stores.test[key], d)
+            self.assertEqual(job.stores.test.get(key), d)
+            self.assertEqual(job.stores.test.get('bs', d), d)


I would suggest another word like 'nonexistent_key' so it's more clear. Do a find/replace because this re-occurs.

Ummm, yes. Oops. :D

I'm going to fix that in a separate commit, because there are a bunch of other occurrences of the same pattern in other test modules.

tests/test_job.py

csadorf · 2019-02-24T22:19:05Z

@vyasr I've addressed all of your comments.

vyasr · 2019-02-25T01:02:44Z

Approving pending the change that Bradley requested.

This is related to signac-flow issue #129 and PR #130, where we decided it makes more sense to break up the data space initialization into signac core and the flow project initialization into flow.

csadorf marked this pull request as ready for review February 19, 2019 21:07

csadorf requested a review from a team as a code owner February 19, 2019 21:07

csadorf added this to the v1.0.0 milestone Feb 20, 2019

vyasr assigned vyasr and csadorf and unassigned vyasr Feb 20, 2019

csadorf requested review from vyasr and mikemhenry February 20, 2019 23:50

csadorf added 10 commits February 21, 2019 13:02

Implement the H5StoreManager class.

694a840

Manages multiple instances of H5Store within a specific directory. Exposed as job.stores and project.stores.

Fix Py27 issue.

ccabe8f

Fix exception handling bug in DictManager.

965e0eb

Raise H5StoreClosedError when trying to flush a closed instance of H5…

0a699fd

…Store.

Employ registry in DictManager for reference count management.

642ab51

Register returned dicts in internal registry to ensure persistence of returned dicts over the lifetime of the manager instance.

Add unit tests for custom data stores.

e2598e7

In addition to testing 'job.data', we also test 'job.stores.test'.

Fix dict persistency bug for job.stores.

7f0aef2

This patch fixes an issue that required users to explicitly hold a reference to each store returned from `job.stores`, otherwise the returned container would be immediatley deleted prior to use.

Fix pickling issue in DictManager.

8c10735

Fix bug causing the DictManager.__delattr__ to fail.

dae9ea9

Implement H5Store.__delattr__ method.

ed4ddde

csadorf force-pushed the feature/h5-store-manager branch from d9f8cb5 to ed4ddde Compare February 21, 2019 18:02

mikemhenry reviewed Feb 21, 2019

View reviewed changes

vyasr requested changes Feb 24, 2019

View reviewed changes

csadorf commented Feb 24, 2019

View reviewed changes

vyasr self-requested a review February 24, 2019 16:02

Update changelog.

ce814b4

bdice requested review from mikemhenry and removed request for mikemhenry February 24, 2019 18:03

bdice reviewed Feb 24, 2019

View reviewed changes

tests/test_job.py Outdated Show resolved Hide resolved

bdice self-requested a review February 24, 2019 18:15

csadorf added 4 commits February 24, 2019 16:44

Update Job.data/stores and Project.data/stores doc strings.

32b1b1b

Improve documentation of DictManager constructor.

eb5bd73

Add unit tests for job.stores implicit initialization.

fa32152

Fix a key name in a job unit test.

f42ed84

vyasr approved these changes Feb 25, 2019

View reviewed changes

csadorf merged commit 66cdb21 into develop Feb 25, 2019

csadorf deleted the feature/h5-store-manager branch February 25, 2019 01:17

tcmoore3 mentioned this pull request Jul 26, 2019

Add function to initialize a test workspace #215

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the H5StoreManager class. #129

Implement the H5StoreManager class. #129

csadorf commented Feb 19, 2019 •

edited

Loading

csadorf commented Feb 20, 2019

mikemhenry left a comment

csadorf commented Feb 21, 2019

vyasr commented Feb 23, 2019

vyasr left a comment

vyasr Feb 24, 2019

csadorf Feb 24, 2019

csadorf Feb 24, 2019

vyasr Feb 24, 2019

csadorf Feb 24, 2019

vyasr Feb 24, 2019

csadorf Feb 24, 2019

vyasr Feb 25, 2019 •

edited

Loading

csadorf left a comment •

edited

Loading

csadorf Feb 24, 2019

csadorf Feb 24, 2019

bdice Feb 24, 2019 •

edited

Loading

csadorf Feb 24, 2019

csadorf Feb 24, 2019

csadorf commented Feb 24, 2019

vyasr commented Feb 25, 2019

Implement the H5StoreManager class. #129

Implement the H5StoreManager class. #129

Conversation

csadorf commented Feb 19, 2019 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

csadorf commented Feb 20, 2019

mikemhenry left a comment

Choose a reason for hiding this comment

csadorf commented Feb 21, 2019

vyasr commented Feb 23, 2019

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

csadorf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf commented Feb 24, 2019

vyasr commented Feb 25, 2019

csadorf commented Feb 19, 2019 •

edited

Loading

vyasr Feb 25, 2019 •

edited

Loading

csadorf left a comment •

edited

Loading

bdice Feb 24, 2019 •

edited

Loading