Repository: bluesky/bluesky-darkframes

Issue:
Fix Resource document creation to make uid unique
Currently, we re-issue the same Resource document multiple times, resulting in Resource documents with colliding UIDs in the database. (Their `run_start` keys are different, though.) For awhile we entertained the idea of letting `uid` be nonunique so long as the pair `(run_start, uid)` was unique. Now we have thought better of that, so the plan is:

* Fix the code that was generating clashing UIDs (bluesky-darkframes).
* Do a database migration to fix the existing documents.
* An add index in suitcase-mongo to ensure that UIDs are unique going forward.

Discussion:
danielballan: Tests pass but documentation build fails in a way that looks important. To be continued.

Patch:
diff --git a/.flake8 b/.flake8
index 5e2e49c..97bf913 100644
--- a/.flake8
+++ b/.flake8
@@ -1,2 +1,10 @@
 [flake8]
-exclude = .git,__pycache__,build,dist,versioneer.py,bluesky_darkframes/_version.py,docs/source/conf.py
+exclude =
+    .git,
+    __pycache__,
+    build,
+    dist,
+    versioneer.py,
+    bluesky_darkframes/_version.py,
+    docs/source/conf.py
+max-line-length = 115
diff --git a/.travis.yml b/.travis.yml
index acc748e..f1cc1e3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -24,7 +24,7 @@ script:
   - coverage run -m pytest  # Run the tests and check for test coverage.
   - coverage report -m  # Generate test coverage report.
   - codecov  # Upload the report to codecov.
-  - flake8 --max-line-length=115  # Enforce code style (but relax line length limit a bit).
+  - flake8  # Enforce code style.
   - make -C docs html  # Build the documentation.
   - set -e
   - pip install doctr
diff --git a/bluesky_darkframes/__init__.py b/bluesky_darkframes/__init__.py
index de2f5fa..ddb3fac 100644
--- a/bluesky_darkframes/__init__.py
+++ b/bluesky_darkframes/__init__.py
@@ -2,6 +2,7 @@ import collections
 import copy
 import logging
 import time
+import uuid
 
 import event_model
 from frozendict import frozendict
@@ -33,8 +34,8 @@ class SnapshotDevice(Device):
         self._describe_configuration = device.describe_configuration()
         self._read = device.read()
         self._read_configuration = device.read_configuration()
-        self._read_attrs = list(device.read())
-        self._configuration_attrs = list(device.read_configuration())
+        self._read_attrs = list(self._read)
+        self._configuration_attrs = list(self._read_configuration)
         self._asset_docs_cache = list(device.collect_asset_docs())
         self._assets_collected = False
 
@@ -65,10 +66,59 @@ class SnapshotDevice(Device):
         if self._assets_collected:
             yield from []
         else:
+            self._assets_collected = True
             yield from self._asset_docs_cache
 
     def stage(self):
         self._assets_collected = False
+        return super().stage()
+
+    def unstage(self):
+        self._remake_docs()
+        return super().unstage()
+
+    def _remake_docs(self):
+        """
+        Avoid re-emitting documents with the same unique identifiers.
+
+        - Make shallow copies of Resource and Datum docs with new unique identifiers.
+        - Update the return value of read() with the new datum_ids.
+        """
+        resources = {}  # map old uid to new uid
+        new_asset_docs_cache = []
+        for name, doc in self._asset_docs_cache:
+            if name == 'resource':
+                new_uid = str(uuid.uuid4())
+                resources[doc['uid']] = new_uid
+                new_doc = doc.copy()
+                new_doc['uid'] = new_uid
+                new_asset_docs_cache.append((name, new_doc))
+            elif name == 'datum':
+                new_doc = doc.copy()
+                old_resource_uid = doc['resource']
+                new_resource_uid = resources[old_resource_uid]
+                new_doc['resource'] = new_resource_uid
+                # Some existing code in other libraries looks for the
+                # {resource_uid}/{integer} pattern in Event documents and uses that
+                # to take a fast path. The changes to Datum proposed in
+                # https://github.com/bluesky/event-model/issues/156
+                # would make this less fragile.
+                old_datum_id = doc['datum_id']
+                if old_datum_id.startswith(f"{old_resource_uid}/"):
+                    _, suffix = old_datum_id.split('/', 1)
+                    new_datum_id = f"{new_resource_uid}/{suffix}"
+                else:
+                    new_datum_id = str(uuid.uuid4())
+                new_doc['datum_id'] = new_datum_id
+                new_asset_docs_cache.append((name, new_doc))
+                # Update the return value of read() to replace the old datum_id
+                # with the new one.
+                for k, v in list(self._read.items()):
+                    if v['value'] == old_datum_id:
+                        self._read[k]['value'] = new_datum_id
+            else:
+                raise BlueskyDarkframesValueError(f"Unexpected name {name}")
+        self._asset_docs_cache = new_asset_docs_cache
 
 
 class _SnapshotShell:
@@ -177,7 +227,7 @@ class DarkFramePreprocessor:
 
     def _evict_old_entries(self):
         now = time.monotonic()
-        for key, (creation_time, snapshot) in list(self._cache.items()):
+        for key, (creation_time, _snapshot) in list(self._cache.items()):
             if now - creation_time > self.max_age:
                 logger.debug("Evicted old snapshot for state %r", key)
                 # Too old. Evict from cache.
diff --git a/docs/source/usage.rst b/docs/source/usage.rst
index 22012a8..7071e1a 100644
--- a/docs/source/usage.rst
+++ b/docs/source/usage.rst
@@ -106,11 +106,17 @@ for whether/how dark frames can be reused.)
    shutter = Shutter(name='shutter', value='open')
 
    def dark_plan(detector):
+       # Restage to ensure that dark frames goes into a separate file.
+       yield from bps.unstage(detector)
+       yield from bps.stage(detector)
        yield from bps.mv(shutter, 'closed')
        yield from bps.trigger(detector, group='darkframe-trigger')
        yield from bps.wait('darkframe-trigger')
        snapshot = bluesky_darkframes.SnapshotDevice(detector)
        yield from bps.mv(shutter, 'open')
+       # Restage.
+       yield from bps.unstage(detector)
+       yield from bps.stage(detector)
        return snapshot
 
 This is boilerplate bluesky and databroker setup not specificially related to
@@ -237,7 +243,7 @@ Here we use a :class:`event_model.RunRouter`.
 .. jupyter-execute::
 
    from bluesky_darkframes import DarkSubtraction
-   from event_model import RunRouter, Filler
+   from event_model import RunRouter
    from suitcase.tiff_series import Serializer
 
    def factory(name, doc):
@@ -246,23 +252,16 @@ Here we use a :class:`event_model.RunRouter`.
        # and then tearing it down when we're done with this run.
        subtractor = DarkSubtraction('det_image')
        serializer = Serializer('live_exported_files/')
-       filler = Filler(db.reg.handler_reg, inplace=False)
-
-       # Here we push the run 'start' doc through.
-       subtractor(name, doc)
-       serializer(name, doc)
-       filler(name, doc)
 
        # And by returning this function below, we are routing all other
        # documents *for this run* through here.
-       def fill_subtract_and_serialize(name, doc):
-           name, doc = filler(name, doc)
+       def subtract_and_serialize(name, doc):
            name, doc = subtractor(name, doc)
            serializer(name, doc)
 
-       return [fill_subtract_and_serialize], []
+       return [subtract_and_serialize], []
 
-   rr = RunRouter([factory])
+   rr = RunRouter([factory], db.reg.handler_reg)
    RE.subscribe(rr);
 
 Now take some data.
diff --git a/requirements.txt b/requirements.txt
index 325ece9..accc8c1 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,4 +1,4 @@
 bluesky
-event-model >=1.13.2
+event-model >=1.14.0
 frozendict
 numpy


Test patch:
diff --git a/bluesky_darkframes/tests/tests.py b/bluesky_darkframes/tests/tests.py
index 590334c..98ddcbb 100644
--- a/bluesky_darkframes/tests/tests.py
+++ b/bluesky_darkframes/tests/tests.py
@@ -5,7 +5,7 @@ import bluesky.plan_stubs as bps
 import bluesky_darkframes
 import bluesky_darkframes.sim
 from bluesky.plans import count
-from event_model import RunRouter, Filler
+from event_model import RunRouter
 from ophyd.sim import NumpySeqHandler
 import pytest
 from suitcase.tiff_series import Serializer
@@ -18,11 +18,15 @@ shutter = bluesky_darkframes.sim.Shutter(name='shutter', value='open')
 
 
 def dark_plan(detector):
+    yield from bps.unstage(detector)
     yield from bps.mv(shutter, 'closed')
+    yield from bps.stage(detector)
     yield from bps.trigger(detector, group='darkframe-trigger')
     yield from bps.wait('darkframe-trigger')
     snapshot = bluesky_darkframes.SnapshotDevice(detector)
+    yield from bps.unstage(detector)
     yield from bps.mv(shutter, 'open')
+    yield from bps.stage(detector)
     return snapshot
 
 
@@ -179,6 +183,37 @@ def test_limit(RE):
     previous_state = state
 
 
+def test_non_colliding_uids(RE):
+    """
+    Tests that when the same Snapshot is used multiple times it issues distinct
+    Resource and Datum documents with non-colliding uids.
+    """
+
+    class LocalException(Exception):
+        ...
+
+    cache = set()
+
+    def check_uniqueness(name, doc):
+        if name == 'datum':
+            key = (name, doc['datum_id'])
+        else:
+            key = (name, doc['uid'])
+        if key in cache:
+            raise LocalException(f"Collision {key}")
+        cache.add(key)
+
+    RE.subscribe(check_uniqueness)
+
+    dark_frame_preprocessor = bluesky_darkframes.DarkFramePreprocessor(
+        dark_plan=dark_plan, detector=det, max_age=100)
+    RE.preprocessors.append(dark_frame_preprocessor)
+
+    RE(count([det]), check_uniqueness)
+    RE(count([det]), check_uniqueness)
+    RE(count([det]), check_uniqueness)
+
+
 @pytest.mark.parametrize('pedestal', [None, 0, 100])
 def test_streaming_export(RE, tmp_path, pedestal):
     """
@@ -193,23 +228,16 @@ def test_streaming_export(RE, tmp_path, pedestal):
             kwargs['pedestal'] = pedestal
         subtractor = bluesky_darkframes.DarkSubtraction('det_image', **kwargs)
         serializer = Serializer(tmp_path)
-        filler = Filler({'NPY_SEQ': NumpySeqHandler}, inplace=False)
-
-        # Here we push the run 'start' doc through.
-        subtractor(name, doc)
-        serializer(name, doc)
-        filler(name, doc)
 
         # And by returning this function below, we are routing all other
         # documents *for this run* through here.
-        def fill_subtract_and_serialize(name, doc):
-            name, doc = filler(name, doc)
+        def subtract_and_serialize(name, doc):
             name, doc = subtractor(name, doc)
             serializer(name, doc)
 
-        return [fill_subtract_and_serialize], []
+        return [subtract_and_serialize], []
 
-    rr = RunRouter([factory])
+    rr = RunRouter([factory], {'NPY_SEQ': NumpySeqHandler})
     RE.subscribe(rr)
 
     dark_frame_preprocessor = bluesky_darkframes.DarkFramePreprocessor(
@@ -232,23 +260,16 @@ def test_no_dark_frames(RE, tmp_path):
         # and then tearing it down when we're done with this run.
         subtractor = bluesky_darkframes.DarkSubtraction('det_image')
         serializer = Serializer(tmp_path)
-        filler = Filler({'NPY_SEQ': NumpySeqHandler}, inplace=False)
-
-        # Here we push the run 'start' doc through.
-        subtractor(name, doc)
-        serializer(name, doc)
-        filler(name, doc)
 
         # And by returning this function below, we are routing all other
         # documents *for this run* through here.
-        def fill_subtract_and_serialize(name, doc):
-            name, doc = filler(name, doc)
+        def subtract_and_serialize(name, doc):
             name, doc = subtractor(name, doc)
             serializer(name, doc)
 
-        return [fill_subtract_and_serialize], []
+        return [subtract_and_serialize], []
 
-    rr = RunRouter([factory])
+    rr = RunRouter([factory], {'NPY_SEQ': NumpySeqHandler})
     RE.subscribe(rr)
 
     # We intentionally 'forget' to set up a dark_frame_preprocessor for this