Make synced collection a hidden package

[kidrahahjo]

Description

Make synced collection package hidden.

Motivation and Context

Resolves #790, resolves #755

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.
• The changes introduced by this pull request are covered by existing or newly introduced tests.
• The package documentation and framework documentation in signac-docs are up to date with these changes.
• I have updated the changelog and added any related issue and pull request numbers for future reference.

[codecov]

Codecov Report

Merging #807 (0279ce0) into next (52ff017) will decrease coverage by 1.31%.
The diff coverage is 78.85%.

@@            Coverage Diff             @@
##             next     #807      +/-   ##
==========================================
- Coverage   86.32%   85.00%   -1.32%     
==========================================
  Files          51       54       +3     
  Lines        4687     4675      -12     
  Branches     1022     1011      -11     
==========================================
- Hits         4046     3974      -72     
- Misses        456      522      +66     
+ Partials      185      179       -6     

Impacted Files	Coverage Δ
signac/_synced_collections/__init__.py	100.00% <ø> (ø)
signac/_synced_collections/_caching.py	0.00% <0.00%> (ø)
signac/_synced_collections/backends/__init__.py	100.00% <ø> (ø)
..._synced_collections/backends/collection_mongodb.py	90.90% <ø> (ø)
...c/_synced_collections/backends/collection_redis.py	65.62% <ø> (ø)
...ac/_synced_collections/backends/collection_zarr.py	88.00% <ø> (ø)
signac/_synced_collections/buffers/__init__.py	100.00% <ø> (ø)
..._collections/buffers/memory_buffered_collection.py	100.00% <ø> (ø)
signac/_synced_collections/data_types/__init__.py	100.00% <ø> (ø)
signac/_synced_collections/data_types/attr_dict.py	100.00% <ø> (ø)
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[bdice]

Awesome start! I suggested a few minor changes.

[bdice]

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.

[kidrahahjo]

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

[bdice]

😕 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

[vyasr]

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).

[bdice]

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.

[kidrahahjo]

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?

[bdice]

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.

[kidrahahjo]

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)

[vyasr]

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.

[vyasr]

@bdice @kidrahahjo I've standardized the docs in a relatively simple way. Let me know what you two think. In the interest of getting 2.0 done. I think we shouldn't belabor the documentation question too much further for now; we can revisit in 2.x, especially once synced_collections is actually a separate, accessible package, but I don't want to block 2.0 over that discussion.

[bdice]

Thanks for those changes @vyasr. This looks fine to me.