Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make synced collection a hidden package #807

Merged
merged 7 commits into from
Sep 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.txt
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Removed
- The ability to call ``Project.workspace``, it is strictly a property now (#752).
- ``ProjectSchema.__call__``, ``ProjectSchema.detect`` (#752).
- The ``--doc-filter`` option for several command line subcommands (#613, #795).
- The public API of the ``synced_collection`` subpackage (#807, #790).

Version 1
=========
Expand Down
108 changes: 0 additions & 108 deletions doc/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -173,111 +173,3 @@ signac.errors module
:members:
:undoc-members:
:show-inheritance:

synced\_collections package
===========================

Data Types
----------

synced\_collections.synced\_collection module
+++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.data_types.synced_collection
:members:
:private-members:
:show-inheritance:

synced\_collections.synced\_dict module
+++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.data_types.synced_dict
:members:
:show-inheritance:

synced\_collections.synced\_list module
+++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.data_types.synced_list
:members:
:show-inheritance:

Backends
--------

synced\_collections.backends.collection\_json module
++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.backends.collection_json
:members:
:show-inheritance:

synced\_collections.backends.collection\_mongodb module
+++++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.backends.collection_mongodb
:members:
:show-inheritance:

synced\_collections.backends.collection\_redis module
+++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.backends.collection_redis
:members:
:show-inheritance:

synced\_collections.backends.collection\_zarr module
++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.backends.collection_zarr
:members:
:show-inheritance:

Buffers
-------

synced\_collections.buffers.buffered\_collection module
+++++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.buffers.buffered_collection
:members:
:private-members:
:show-inheritance:

synced\_collections.buffers.file\_buffered\_collection module
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.buffers.file_buffered_collection
:members:
:show-inheritance:

synced\_collections.buffers.serialized\_file\_buffered\_collection module
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.buffers.serialized_file_buffered_collection
:members:
:show-inheritance:

synced\_collections.buffers.memory\_buffered\_collection module
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.buffers.memory_buffered_collection
:members:
:show-inheritance:

Miscellaneous Modules
---------------------

synced\_collections.utils module
++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.utils
:members:
:show-inheritance:

synced\_collections.validators module
+++++++++++++++++++++++++++++++++++++

.. automodule:: signac.synced_collections.validators
:members:
:show-inheritance:
6 changes: 3 additions & 3 deletions signac/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
"""

from . import contrib, errors, sync, testing, warnings
from ._synced_collections.backends.collection_json import (
BufferedJSONAttrDict as JSONDict,
)
from .contrib import Project, TemporaryProject, get_job, get_project, init_project
from .core.h5store import H5Store, H5StoreManager
from .diff import diff_jobs
from .synced_collections.backends.collection_json import (
BufferedJSONAttrDict as JSONDict,
)
from .version import __version__

# Alias some properties related to buffering into the signac namespace.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class BufferedCollection(SyncedCollection):

- :meth:`~._load_from_buffer`: Loads data while in buffered mode and returns
it in an object satisfying
:meth:`~signac.synced_collections.data_types.synced_collection.SyncedCollection.is_base_type`.
:meth:`~signac._synced_collections.data_types.synced_collection.SyncedCollection.is_base_type`.
The default behavior is to simply call
:meth:`~._load_from_resource`.
- :meth:`~._save_to_buffer`: Stores data while in buffered mode. The default behavior
Expand Down Expand Up @@ -159,7 +159,7 @@ def _load_from_buffer(self):
-------
Collection
An equivalent unsynced collection satisfying
:meth:`~signac.synced_collections.data_types.synced_collection.SyncedCollection.is_base_type` that
:meth:`~signac._synced_collections.data_types.synced_collection.SyncedCollection.is_base_type` that
contains the buffered data. By default, the buffered data is just the
data in the resource.

Expand Down
2 changes: 1 addition & 1 deletion signac/contrib/hashing.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import hashlib
import json

from ..synced_collections.utils import SyncedCollectionJSONEncoder
from .._synced_collections.utils import SyncedCollectionJSONEncoder

# We must use the standard library json for exact consistency in formatting

Expand Down
23 changes: 12 additions & 11 deletions signac/contrib/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
from threading import RLock
from typing import FrozenSet

from ..core.h5store import H5StoreManager
from ..sync import sync_jobs
from ..synced_collections.backends.collection_json import (
from .._synced_collections.backends.collection_json import (
BufferedJSONAttrDict,
JSONAttrDict,
json_attr_dict_validator,
)
from ..synced_collections.errors import KeyTypeError
from .._synced_collections.errors import KeyTypeError
from ..core.h5store import H5StoreManager
from ..sync import sync_jobs
from .errors import DestinationExistsError, JobsCorruptedError
from .hashing import calc_id
from .utility import _mkdir_p
Expand Down Expand Up @@ -403,7 +403,7 @@ def statepoint(self):
you can access a dict copy of the state point by calling it, e.g.
``sp_dict = job.statepoint()`` instead of ``sp = job.statepoint``.
For more information, see
:class:`~signac.synced_collections.backends.collection_json.JSONAttrDict`.
:class:`~signac._synced_collections.backends.collection_json.JSONAttrDict`.
Comment on lines 405 to +406
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class reference won't work because we're removing the synced collection docs. I wonder if we should add docs for the publicly imported name signac.JSONDict and point to that instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the docs for JSONDict (which is buffered) and JSONAttrDict be different?

Copy link
Member

@bdice bdice Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 This is tricky. We want to just point to public docs for a standalone synced collections package, but if we make synced collections hidden, we can't point to these docs.

Maybe this entire approach is flawed and we should leave signac.synced_collections as a public subpackage until we make synced_collections standalone. Maybe this is a signac 3.0 task and we should leave a public subpackage in 2.0.

cc: @glotzerlab/signac-committers

Copy link
Contributor

@vyasr vyasr Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment on the corresponding issue. Even if synced_collections stays public, I think pointing to a specific class here is a flawed approach. From a user's perspective, do they need any information other than the fact that this attribute is a dict-like object? If we really want to explain the syncing, I would say that we could point to the parent SyncedDict class, but IMO that's exposing implementation details because synchronization is transparent.

There is only one minor exception that comes to my mind immediately, which is that the document currently supports buffering on a per-document basis:

with job.doc.buffered:
    # All I/O will be buffered in this block

but TBH I'm not sure that's something that we really need to promise at the signac level. The signac.buffered API should be the only entrypoint for buffering for signac users IMO:

with signac.buffered():
    # Buffer everything

since that abstracts over the details of specific backends and all instances. That abstraction guarantees that in a world where signac starts supporting different backends, users still have a single switch in the API to toggle when reads and writes are maximally buffered, and it's up to the implementation of signac to actually perform that buffering to the best of its ability. So I think that ultimately the only API promise (and therefore all that we should document) is that [job|project].[sp|doc] are MutableMappings, and perhaps that they support attribute-based access. We shouldn't link to a type because we don't actually promise the type of the object (again, see #790 (comment) for why).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this approach. Thanks @vyasr. @kidrahahjo does this make sense on what to change? If not, I can take a look at changing it myself and push to this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understood this, we do not need to expose any of the implementation details so we can either not specify the type or use MutableMappings (which is another way of saying its a dict like object) instead?


See :ref:`signac statepoint <signac-cli-statepoint>` for the command line equivalent.

Expand All @@ -415,8 +415,9 @@ def statepoint(self):

Returns
-------
dict
Returns the job's state point.
MutableMapping
Returns the job's state point. Supports attribute-based access to
dict keys.
"""
with self._lock:
if self._statepoint_requires_init:
Expand Down Expand Up @@ -481,8 +482,8 @@ def document(self):

Returns
-------
:class:`~signac.JSONDict`
The job document handle.
MutableMapping
The job document handle. Supports attribute-based access to dict keys.

"""
with self._lock:
Expand Down Expand Up @@ -522,8 +523,8 @@ def doc(self):

Returns
-------
:class:`~signac.JSONDict`
The job document handle.
MutableMapping
The job document handle. Supports attribute-based access to dict keys.

"""
return self.document
Expand Down
2 changes: 1 addition & 1 deletion signac/contrib/migration/v1_to_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@

import os

from signac._synced_collections.backends.collection_json import BufferedJSONAttrDict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should replace all references to signac._synced_collections.backends.collection_json.BufferedJSONAttrDict with signac.JSONDict, which is an alias that we publicly expose. Then the dependence on synced collections is more isolated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also use JSONAttrDict in some parts of our code. Since I am not super familiar with the code I am not sure if it's safe to replace the use of JSONAttrDict with BufferedJSONAttrDict (as signac.JSONDict is a BufferedJSONAttrDict)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say let's leave this as is for now. If and when we get around to making synced_collections a separate published package it'll get easier since we'll be referencing an external, public package.

from signac.common import configobj
from signac.common.config import _get_project_config_fn
from signac.contrib.project import Project
from signac.synced_collections.backends.collection_json import BufferedJSONAttrDict

from .v0_to_v1 import _load_config_v1

Expand Down
10 changes: 5 additions & 5 deletions signac/contrib/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from tempfile import TemporaryDirectory
from threading import RLock

from .._synced_collections.backends.collection_json import BufferedJSONAttrDict
from ..common.config import (
Config,
_get_project_config_fn,
Expand All @@ -28,7 +29,6 @@
)
from ..core.h5store import H5StoreManager
from ..sync import sync_projects
from ..synced_collections.backends.collection_json import BufferedJSONAttrDict
from ..version import SCHEMA_VERSION, __version__
from ._searchindexer import _SearchIndexer
from .errors import (
Expand Down Expand Up @@ -301,8 +301,8 @@ def document(self):

Returns
-------
:class:`~signac.synced_collections.backends.collection_json.BufferedJSONAttrDict`
The project document.
MutableMapping
The project document. Supports attribute-based access to dict keys.

"""
with self._lock:
Expand Down Expand Up @@ -334,8 +334,8 @@ def doc(self):

Returns
-------
:class:`~signac.synced_collections.backends.collection_json.BufferedJSONAttrDict`
The project document.
MutableMapping
The project document. Supports attribute-based access to dict keys.

"""
return self.document
Expand Down
2 changes: 1 addition & 1 deletion signac/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
# exceptions that are relevant beyond a single module. This top-level errors
# module is used to expose user-facing exception classes.

from ._synced_collections.errors import InvalidKeyError, KeyTypeError
from .common.errors import ConfigError
from .contrib.errors import (
DestinationExistsError,
Expand All @@ -16,7 +17,6 @@
WorkspaceError,
)
from .core.errors import Error, H5StoreAlreadyOpenError, H5StoreClosedError
from .synced_collections.errors import InvalidKeyError, KeyTypeError


class SyncConflict(Error, RuntimeError):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_buffered_mode.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from test_project import TestProjectBase

import signac
from signac.synced_collections.errors import BufferedError
from signac._synced_collections.errors import BufferedError

PYPY = "PyPy" in platform.python_implementation()

Expand Down
2 changes: 1 addition & 1 deletion tests/test_numpy_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import pytest
from test_project import TestProjectBase

from signac.synced_collections.numpy_utils import NumpyConversionWarning
from signac._synced_collections.numpy_utils import NumpyConversionWarning

try:
import numpy # noqa
Expand Down
4 changes: 2 additions & 2 deletions tests/test_synced_collections/synced_collection_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@

import pytest

from signac._synced_collections import SyncedCollection
from signac._synced_collections.numpy_utils import NumpyConversionWarning
from signac.errors import KeyTypeError
from signac.synced_collections import SyncedCollection
from signac.synced_collections.numpy_utils import NumpyConversionWarning

PYPY = "PyPy" in platform.python_implementation()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from attr_dict_test import AttrDictTest, AttrListTest
from test_json_collection import JSONCollectionTest, TestJSONDict, TestJSONList

from signac.synced_collections.backends.collection_json import (
from signac._synced_collections.backends.collection_json import (
BufferedJSONAttrDict,
BufferedJSONAttrList,
BufferedJSONDict,
Expand All @@ -21,7 +21,7 @@
MemoryBufferedJSONDict,
MemoryBufferedJSONList,
)
from signac.synced_collections.errors import BufferedError, MetadataError
from signac._synced_collections.errors import BufferedError, MetadataError


class BufferedJSONCollectionTest(JSONCollectionTest):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_synced_collections/test_json_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from attr_dict_test import AttrDictTest, AttrListTest
from synced_collection_test import SyncedDictTest, SyncedListTest

from signac.synced_collections.backends.collection_json import (
from signac._synced_collections.backends.collection_json import (
JSONAttrDict,
JSONAttrList,
JSONDict,
Expand Down
2 changes: 1 addition & 1 deletion tests/test_synced_collections/test_mongodb_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import pytest
from synced_collection_test import SyncedDictTest, SyncedListTest

from signac.synced_collections.backends.collection_mongodb import (
from signac._synced_collections.backends.collection_mongodb import (
MongoDBDict,
MongoDBList,
)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_synced_collections/test_redis_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import pytest
from synced_collection_test import SyncedDictTest, SyncedListTest

from signac.synced_collections.backends.collection_redis import RedisDict, RedisList
from signac._synced_collections.backends.collection_redis import RedisDict, RedisList

try:
import redis
Expand Down
8 changes: 4 additions & 4 deletions tests/test_synced_collections/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@

import pytest

from signac.synced_collections import SyncedList
from signac.synced_collections.backends.collection_json import JSONDict
from signac.synced_collections.numpy_utils import NumpyConversionWarning
from signac.synced_collections.utils import (
from signac._synced_collections import SyncedList
from signac._synced_collections.backends.collection_json import JSONDict
from signac._synced_collections.numpy_utils import NumpyConversionWarning
from signac._synced_collections.utils import (
AbstractTypeResolver,
SyncedCollectionJSONEncoder,
)
Expand Down
4 changes: 2 additions & 2 deletions tests/test_synced_collections/test_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
# This software is licensed under the BSD 3-Clause License.
import pytest

from signac.synced_collections.errors import InvalidKeyError, KeyTypeError
from signac.synced_collections.validators import (
from signac._synced_collections.errors import InvalidKeyError, KeyTypeError
from signac._synced_collections.validators import (
json_format_validator,
no_dot_in_key,
require_string_key,
Expand Down
Loading