Feature/synced collections #484
Conversation
Adds a generic framework for synced collections to signac. The abstract SyncedCollection defines the interface for any Collection-like object (as defined by Python's collections.abc.Collection) that is synced with some form of persistent storage. This concept generalizes the existing _SyncedDict and SyncedList classes by implementing a common framework for these. Critically, this framework decouples the act of saving to and loading from the backend from the recursive conversion between a SyncedCollection and its base data type (e.g. a SyncedDict to a dict and vice versa), the latter of which is necessary to ensure proper synchronization of nested collections. This decoupling makes it much easier to enable different back ends for a given data structure without having to modify the synchronization logic.
Adds Redis, Zarr, and MongoDB backends.
SyncedCollections can now arbitrary validators to input data to ensure that data conforms to a desired specification. This layer can be employed to ensure, for instance, that all inputs are valid JSON.
Adds the ability to buffer I/O in SyncedCollections using an in-memory cache to store data.
Reorganizes synced collections into their own subpackage and move all corresponding tests into a subdirectory for easier identification.
Cleans up and standardizes the testing architecture so that all backends can easily apply the exact same set of tets using a few minor extra pieces of information. Prevents duplicate execution of tests upon import, while also adding a large number of additional tests for backends that were not previously tested.
Rename various methods and remove unnecessary ones from the public API. Standardize internal APIs and simplify the implementation of new backends by automating more subclass behaviors. Improve constructors to enable positional arguments. Improve interfaces for various backends by making it easier for the user to specify and access the precise storage mechanisms.
…ion (#446) Makes the SyncedCollection framework adhere to black, isort, flake8, pydocstyle, and mypy requirements while adding sufficiently detailed documentation to all classes.
Simplifies and standardizes file buffering implementation. Adds extra tests for SyncedAttrDict.update and simplifies its implementation to use _update.
Optimize various aspects of the SyncedCollection framework, including validators, abstract base class type lookups, and the core data structures.
MongoDB and Redis will no longer be silently skipped on CI, so any changes that break support for those will be immediately discovered.
Operations that modify SyncedCollections (and their subclasses) now acquire object-specific mutexes prior to modification in order to guarantee consistency between threads. Thread safety can be enabled or disabled by the user.
The new buffering mode is a variant on the old one that avoids unnecessary encoding, decoding, and in-memory updating by directly sharing memory between various collections accessing the same file in the buffer. This direct sharing allows all changes to be automatically persisted, avoiding any cache inconsistencies without the high overhead of JSON serialization for every single modification.
Completes TODO items scattered throughout the code base and removes a number of outdated ones that have already been addressed.
In addition to synced collections being thread safe individually, while in buffered mode the buffer accesses also have to be made thread safe for multithreaded operation to be safe. This pull request introduces sufficient locking mechanisms to support thread-safe reads from and writes to the buffers.
Unifies the implementation of the two different file buffering modes as much as possible using a shared base class. In addition, this fixes a couple of issues with the thread-safe buffering solution in #468 that only show up on PyPy where execution is fast enough to stress-test multithreaded execution. It also reduces thread locking scopes to the minimum possible.
Removes usage of contextlib.contextmanager and replaces it with custom context classes implementing the context manager protocol. The contextlib decorator has measurable overhead that these pre-instantiated context managers avoid. Furthermore, many of the context managers in synced collections follow a very similar counter-based pattern that is now generalized and shared between them.
…lename of JSONDict is safe.
…source arguments (e.g. filename for a JSONDict) is valid.
The old JSONDict and SyncedAttrDict classes are replaced with the new ones from the SyncedCollections framework. The new classes are now used for the Job's statepoint and document attributes as well as the Project document. The state point is now stored in the new _StatePointDict class, which extends the new JSONDict class to afford greater control over saving and loading of data. Much of the internals of the Job class have also been simplified, with most of the complex logic for job migration and validation when the state point changes now contained within the _StatePointDict. * Replace old JSONDict with new BufferedJSONDict. * Verify that replacing BufferedJSONDict with MemoryBufferedJSONDict. * Remove largely redundant _reset_sp method. * Remove single-use internal functions in Job to reduce surface area for SyncedCollection integration. * Move logic from _init into init. * Working implementation of statepoint using new SyncedCollection. * Remove _check_manifest. * Expose loading explicitly to remove the need for internal laziness in the StatepointDict. * Simplify the code as much as possible by inlining move method and catching the correct error. * Improve documentation of context manager for statepoint loading. * Replace MemoryBufferedJSONDict in Project for now. * Add documentation of why jobs must be stored as a list in the statepoint. * Address PR comments. * Add back import. * Ensure _StatepointDict is always initialized in constructor. * Change _StatepointDict to validate id on load. * Refactor error handling into _StatepointDict class. * Update docstrings. * Update comment. * Fix some docstrings. * Remove redundant JobsCorruptedError check. * Rewrite reset_statepoint to not depend on creating another job. * Reduce direct accesses of internal attributes and do some simplification of the code. * Reraise errors in JSONCollection. * Change reset to require a non-None argument and to call _update internally. * Add reset_data method to provide clear access points of the _StatepointDict for the Job. * Create new internal method for handling resetting. * Move statepoint resetting logic into the statepoint object itself. * Stop accessing internal statepoint filename attribute directly and rely on validation on construction. * Make statepoint thread safe. * Some minor cleanup. * Remove now unnecessary protection of the filename key. * Explicitly document behavior of returning None from _load_from_resource. * Apply suggestions from code review Co-authored-by: Bradley Dice <[email protected]> * Rename SCJSONEncoder to SyncedCollectionJSONEncoder. * Only access old id once. * Move lazy attribute initialization into one location. * Address PR requests that don't cause any issues. * Remove the temporary state point file backup. * Make as many old buffer tests as possible. * Reset buffer size after test. * Last changes from PR review. Co-authored-by: Bradley Dice <[email protected]>
|
I ran benchmarks and profiles with this command: My observations are that this PR is about 2x slower than Synced Collections (this PR)
master (close to v1.6.0)
The slowdown is coming from these two places. Correcting these two problems would make the new code just as fast as the old code, as far as I can tell.
|
|
@bdice thanks for doing some benchmarking. I'll go through your comments later, for now here are my thoughts regarding the two points you brought up on performance.
|
This patch removes attribute-based access to synced dictionaries. This logic is moved to a new `AttrDict` mixin class that can be inherited by any other subclasses if this feature is desired. * Simplify definition of __setattr__ by relying on complete list of protected keys. * Move all attribute-based access to a separate mixin class AttrDict. * Rename SyncedAttrDict to SyncedDict. * Move synced_attr_dict to synced_dict. * Remove attribute-based access from existing backend dict types and add new validator to check string keys. * Isolate deprecated non-str key handling to the _convert_key_to_str json validator. * Add tests of the AttrDict behavior. * Use new attrdict based classes in signac and make all tests pass. * Remove support for inheriting protected attributes, they must be defined by the class itself. * Change initialization order to call super first wherever possible. * Address PR comments. * Address final round of PR comments.
Isolate all numpy logic into a single utility file so that handling of numpy arrays can be standardized. Also substantially improves test coverage, testing a large matrix of different numpy dtypes and shapes with different types of synced collections. The testing framework as a whole is refactored to simplify the code and reduce the amount of boilerplate required to add the new numpy tests. * Initial pass at isolating all numpy logic to a single file. * Use pytest to generate temporary directories. * Stop saving backend kwargs as class variables. * Remove most references to class-level variables in tests. * Remove the _backend_collection variable. * Remove unnecessary autouse fixtures. * Start adding comprehensive tests for numpy conversion. * Make sure locks are released even if saving fails. * Add tests of vector types and add tests for SyncedList as well as SyncedDict. * Use pytest to parametrize numpy types instead of looping manually. * Unify vector and scalar tests. * Stop testing squeezing and just test the relevant shapes directly. * Add test of reset. * Move numpy tests back to main file. * Remove _conert_numpy_scalar, which performed undesirable squeezing of 1-element 1d arrays, and replace usage with _convert_numpy. * Separate numpy testing into separate functions and limit supported behavior to only the necessary use cases. * Add a warning when implicit numpy conversion occurs. * Update changelog. * Address all PR comments aside from numpy conversion in type resolver. * Catch numpy warnings in signac numpy integration tests. * Support older versions of numpy for random number generation. * Fix isort issue introduced by rebase. * Address PR comments. * Allow AbstractTypeResolver to perform arbitrary preprocessing, delegating the numpy-specialized code to the caller and making it less confusing. * Add missing call to _convert_numpy. * Set NUMPY_SHAPES for MongoDB tests when numpy is not present.
* Remove add_validators classmethod and instead require validators to be defined at class definition time, preventing one application from modifying validation process for others and giving a standard means to completely override parent validators in parents. * Change type in docstring.
The extra work performed by os.path.join can be slow, so this PR replaces it with direct string concatenation of os.sep. * Don't use os.path.join where not needed. * Update signac/contrib/job.py Co-authored-by: Bradley Dice <[email protected]>
Define a single validator for JSONAttrDict classes that combines the logic of other validators while reducing collection traversal costs. Also switch from converting numpy arrays to just bypassing the resolver's cache for them. * Use single separate validator for state points for performance. * Remove preprocessor from type resolver and instead use a blocklist that prevents caching data for certain types. * Reorder resolvers to optimize performance. * Make sure not to include strings as sequences. * Move state point validator to collection_json and use for all JSONAttrDict types. * Make sure to also check complex types. * Add back missing period lost during stash merge. * Address review comments.
Since the most common operation on protected keys is to check if some key is within the list of protected keys, this patch changes the `_PROTECTED_KEYS` attribute to a set for faster O(1) membership checks. * Switch protected keys from a sequence to a set for faster containment checks. * Change evaluation order of checks in setattr. * Address PR comments.
|
@bdice here's what your benchmark looks like on the last few optimizations. I used timeit with 10 iterations to get rid of any profiling overhead and get a reasonable estimate of average performance ( master: 9.46 ms ± 809 µs per loop (mean ± std. dev. of 7 runs, 10 loops each) If your benchmarks on #497 still hold (I suspect that it will still help, but less since validation is now much faster) we should get pretty close to the performance of master. |
* Defer state point initialization when lazy loading. * Allow validation to be disabled in SyncedCollection._update. * Unify reset_statepoint logic across methods. * Revert validation-related changes. * Add test, fix bug. * Update tests/test_job.py
* Add a resoler to fast-exit _from_base for non-collection types. * Optimize synced collection init method. * Remove validators property and access internal variable directly. * Add early exit for _convert_array. * Use locally cached root var instead of self._root. * Remove superfluous duplicate check.
* Add option to trust the data source during _update. * Skip validation if JSON is valid. * Fix pre-commit. * Rename trust_source to _validate. * Set _validate=False.
|
@bdice here's an updated set of benchmarks, including #512 both with and without #514: Benchmarks without #514master: 9.46 ms ± 809 µs per loop (mean ± std. dev. of 7 runs, 10 loops each) Benchmarks with #514master: 9.27 ms ± 3.14 ms per loop (mean ± std. dev. of 7 runs, 10 loops each) So #514 doesn't really affect performance much here. However, all of these numbers are for running the benchmark the very first time through. If I rerun the benchmark on the same project (I'm in a Jupyter notebook, so this means not resetting the kernel and not calling |
Description
This pull request should not be squashed upon merge.
This pull request adds the
synced_collectionssubpackage, which introduces a newSyncedCollectionabstract base class defining a standardized interface for acollections.abc.Collection-like object that must be transparently synchronized with some underlying resource. These collections are designed to replace the_SyncedDict,SyncedAttrDict, andJSONDictclasses with a new framework of classes that make it possible to easily interchange different data types (aside from dictionaries) and different resources (aside from JSON files). The new classes generalize the validation process so that different data types and/or resource types can specify the exact validation required. The new classes also generalize the buffering protocol for synced collections so that new buffering methods can be easily defined and interchanged. In addition, the new classes are more performant than the old versions, with the added bonus of thread-safety in parallel environments.All of this is implemented via extensive inheritance. The new code is unfortunately substantially more complicated than the old code, but the separation of concerns is much clearer with the new classes and the new code is much more easily extensible. There is much less bleeding between the responsibilities of the Job class and the state point as well, while the new
BufferedJSONDictis a drop-in replacement for the oldJSONDictclass in the Job and Project documents.Breaking change: There is one very minor breaking change introduced by this PR. The
force_writeoption in buffered mode is not supported by the new code. I have added a compatibility layer so that the argument will be processed, so it's not an API break, but it is a minor change in behavior because it will not actually force writing. This feature is extremely minor, and it's not used by anyone I'm aware of, so I'm comfortable with making this change within a minor release. As a result, I don't think that this PR requires a new major release of signac.Documentation: Should I write up something for signac-docs? I started writing this up, but a lot of the associated documentation is internal-facing or at least for the purpose of others who wish to extend these classes. From signac's perspective, the existing documentation of the job/project document and state point already cover the necessary points. I added synced collections to the API documentation, and the docstrings within these classes are extensive.
Motivation and Context
Resolves #234
Resolves #196
Resolves #397
Resolves #316
Resolves #465
Resolves #249
Resolves #383
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary: