Repository: bluesky/bluesky
Pull request #1345 (2020-05-01 00:34:52)

verify nd step for scan_nd back compatibility
verify the signature of the per_step function meets requirements for back compatibility from 1.5.5 to 1.6.0.

in discussions with @tacaswell 

still to be tested. 


tacaswell: I think the failures due to the tests being strict about the message in the exception:

https://github.com/bluesky/bluesky/blob/e5de942d14e723e843291a1a6cb989c28dd9b9fc/bluesky/tests/test_plans.py#L254-L262

That pattern match will need to be updated to new exception text.
lsammut: > I think the failures due to the tests being strict about the message in the exception:
> 
> https://github.com/bluesky/bluesky/blob/e5de942d14e723e843291a1a6cb989c28dd9b9fc/bluesky/tests/test_plans.py#L254-L262
> 
> That pattern match will need to be updated to new exception text.

Hi @tacaswell, 
yes I noticed this too. If the new message is ok I'll update the test accordingly. 
danielballan: The new error message looks great—go for it! :-)

Thanks for fixing this, @lsammut.
lsammut: @tacaswell @danielballan 
I've added some nd_per_step specific tests and updated the pattern match. The tests shouldn't fail anymore. 

Patch:
diff --git a/bluesky/plans.py b/bluesky/plans.py
index 6e4fa1b2..16a6881f 100644
--- a/bluesky/plans.py
+++ b/bluesky/plans.py
@@ -930,9 +930,26 @@ def scan_nd(detectors, cycler, *, per_step=None, md=None):
                 if name is not None:
                     if name != p_name:
                         return False
-                # it is any extra arguements, check that they do not have
+                # if there are any extra arguments, check that they have a default
                 else:
-                    if p.kind is p.VAR_KEYWORD or p.kind is p.VAR_KEYWORD:
+                    if p.kind is p.VAR_KEYWORD or p.kind is p.VAR_POSITIONAL:
+                        continue
+                    if p.default is p.empty:
+                        return False
+
+            return True
+
+        def _verify_nd_step(sig):
+            if len(sig.parameters) < 3:
+                return False
+            for name, (p_name, p) in zip_longest(['detectors', 'step', 'pos_cache'], sig.parameters.items()):
+                # this is one of the first 3 positional arguements, check that the name matches
+                if name is not None:
+                    if name != p_name:
+                        return False
+                # if there are any extra arguments, check that they have a default
+                else:
+                    if p.kind is p.VAR_KEYWORD or p.kind is p.VAR_POSITIONAL:
                         continue
                     if p.default is p.empty:
                         return False
@@ -941,6 +958,9 @@ def scan_nd(detectors, cycler, *, per_step=None, md=None):
 
         if sig == inspect.signature(bps.one_nd_step):
             pass
+        elif _verify_nd_step(sig):
+            # check other signature for back-compatibility
+            pass
         elif _verify_1d_step(sig):
             # Accept this signature for back-compat reasons (because
             # inner_product_scan was renamed scan).
@@ -958,9 +978,10 @@ def scan_nd(detectors, cycler, *, per_step=None, md=None):
                 return (yield from user_per_step(detectors, motor, step))
             per_step = adapter
         else:
-            raise TypeError("per_step must be a callable with the signature "
+            raise TypeError("per_step must be a callable with the signature \n "
                             "<Signature (detectors, step, pos_cache)> or "
-                            "<Signature (detectors, motor, step)>.")
+                            "<Signature (detectors, motor, step)>. \n"
+                            "per_step signature received: {}".format(sig))
     pos_cache = defaultdict(lambda: None)  # where last position is stashed
     cycler = utils.merge_cycler(cycler)
     motors = list(cycler.keys)


Test patch:
diff --git a/bluesky/tests/test_plans.py b/bluesky/tests/test_plans.py
index d9ed70be..b44ad258 100644
--- a/bluesky/tests/test_plans.py
+++ b/bluesky/tests/test_plans.py
@@ -1,5 +1,6 @@
 from distutils.version import LooseVersion
 import pytest
+import inspect
 from bluesky.tests.utils import DocCollector
 import bluesky.plans as bp
 import bluesky.plan_stubs as bps
@@ -213,6 +214,9 @@ def _good_per_step_factory():
     def per_step_kwargs(detectors, motor, step, **kwargs):
         yield from bps.null()
 
+    def per_nd_step(detectors, post_cache, *args, **kwargs):
+        yield from bps.null()
+
     return pytest.mark.parametrize(
         "per_step",
         [per_step_old, per_step_extra, per_step_exact, per_step_kwargs],
@@ -242,6 +246,15 @@ def _bad_per_step_factory():
     def per_step_only_args(*args):
         "no body"
 
+    def per_nd_step_extra(detectors, step, pos_cache, extra_no_dflt):
+        "no body"
+
+    def per_nd_step_bad_pos(detectors, step, pos_cache, *, extra_no_dflt):
+        "no body"
+
+    def all_wrong(a, b, c=None, *args, d=None, g, **kwargs):
+        "no body"
+
     return pytest.mark.parametrize(
         "per_step",
         [too_few, too_many, extra_required_kwarg, wrong_names, per_step_only_args],
@@ -251,12 +264,15 @@ def _bad_per_step_factory():
 
 @_bad_per_step_factory()
 def test_bad_per_step_signature(hw, per_step):
+    sig = inspect.signature(per_step)
+    print(f'*** test bad_per_step {sig} ***\n')
     with pytest.raises(
         TypeError,
         match=re.escape(
-            "per_step must be a callable with the signature "
+           "per_step must be a callable with the signature \n "
             "<Signature (detectors, step, pos_cache)> or "
-            "<Signature (detectors, motor, step)>."
+            "<Signature (detectors, motor, step)>. \n"
+            "per_step signature received: {}".format(sig)
         ),
     ):
         list(bp.scan([hw.det], hw.motor, -1, 1, 5, per_step=per_step))