Add support for distributed Cameras #80

AnthonyHorton · 2018-10-05T05:33:55Z

Addressing Issue panoptes/POCS#546 by adding scripts and wrappers to enable distributed control of camera arrays using the Pyro4 library.

wtgee

Whew! That was a long review which means that was even longer for you!

Thanks for getting this moved over. Submitting just as comment for now but can approve if you would rather merge before vacation.

Seems like there is a bit we could do to eliminate code duplication between here and POCS, but I'm happy to do that after the fact.

wtgee · 2018-10-05T10:14:29Z

conf_files/pyro_camera.yaml

+# Contains a default configuration with a simulated camera and focuser for testing
+# and example configurations for other camera and focusers (commented out).
+#
+# Do not edit this file, instead copy one of the examples to pyro_camera_local.yaml


I almost want this at the top and in all caps. And it will probably still happen. :)

A compromise, I've made it all caps but didn't move it :-)

wtgee · 2018-10-05T10:16:06Z

huntsman/camera/__init__.py

+from pocs.utils import logger as logger_module
+
+
+def list_connected_cameras():


This isn't different, right? I realize you were probably just copying straying over, but let's just remove from here as the other is still available to us via POCS.

Yes, this entire file is just copied over verbatim from the branch at panoptes/POCS. A fair bit of unnecessary code duplication that I hadn't made any attempt to reduce yet. Replacing this with an import now.

wtgee · 2018-10-05T10:16:48Z

huntsman/camera/__init__.py

+        host (str, optional): hostname or IP address of the name server host. If not given
+            will attempt to locate the name server via UDP network broadcast.
+        logger (logging.Logger, optional): logger to use for messages, if not given will
+            ise the root logger.


wtgee · 2018-10-05T10:18:47Z

huntsman/camera/__init__.py

+            ise the root logger.
+
+    Returns:
+        dict: Dictionary of distributed camera name, URI pairs


maybe the word "detected" in here?

wtgee · 2018-10-05T10:27:25Z

huntsman/camera/__init__.py

+
+                cameras[cam_name] = cam
+
+    # Distributed camera creation


As far as I can tell lines 220-228 are the only lines different from what would be in POCS. Would it work to move these lines into create_distributed_cameras (maybe with rename to create_distributed_cameras_from_config) and then call the the functions sequentially? Would have to deal with the primary camera preference in some fashion but you already assume here that the distributed should override the local.

Would just be nice to reduce all this to only what we need separately.

Well, these few lines plus a small change about how the default primary camera gets chosen (I can't assume that there is a camera named Cam00 so I sort the camera names and choose the first one). But otherwise it is identical to the POCS version.

Yes, it was the awkwardness of keeping track of whether a primary camera has been selected yet that stopped me from making these two entirely separate functions in the first place. I'll have a think about how to work around that.

It doesn't really override a local primary camera with a distributed one, as there will be at most one primary camera specified in the config file so the distributed camera creation will only set the primary camera is the local camera creation hasn't.

OK, reduced code duplication a fair bit by importing pocs.camera.create_cameras_from_config to handle the local camera creation.

wtgee · 2018-10-05T11:11:35Z

huntsman/utils/pyro/__init__.py

+    except errors.NamingError:
+        if not host:
+            # Not given an hostname or IP address. Will attempt to work it out.
+            host = get_own_ip(verbose=True)


Can we just run on the localhost if none is given? Or is auto-detection the assumed usage?

Running on localhost is 'special' for Pyro servers (including the name server), it disable the broadcast responder so they can't be reached from other machines on the network. Good for testing with simulators, but not a very useful default otherwise.

I see the auto detection as a convenience, really. It's probably not 100% reliable for complicated networking configurations where the machine has multiple IP address. For operational systems with fixed IPs it would be better to set the name server host manually via config file.

wtgee · 2018-10-05T11:12:06Z

huntsman/utils/pyro/__init__.py

+
+        if port:
+            port = int(port)
+        else:


else seems unnecessary.

It does. The only effect here is to convert a value of port that evaluates to False (e.g. 0) into None. I'm not sure why I would want that to happen, though.

wtgee · 2018-10-05T11:12:37Z

huntsman/utils/pyro/__init__.py

+    camera servers should be started after the name server, but before POCS.
+
+    Args:
+        ignore_local (bool, optional): If True use the default $POCS/conf_files/pyro_camera.yaml


s/$POCS/$HUNTSMAN_POCS/

I don't see this change.

Fixed another instance of the same thing and missed this one!

wtgee · 2018-10-05T11:13:19Z

huntsman/utils/pyro/__init__.py

+        print('Starting request loop... (Control-C/Command-C to exit)')
+        try:
+            daemon.requestLoop()
+        finally:


Do you need to catch the KeyboardInterrupt?

I tried doing that, but it didn't work as I expected. The except block didn't get executed. So I just put all the cleanup in finally and that did work.

Maybe:

try: daemon... except KeyboardInterrupt: logger.info('Stopping name server') finally: # Cleanup

Not necessary though.

That would only be so that we could add an additional except Exception so we could capture when it fails for some other reason. But again, not necessary for this PR.

scripts/pyro_name_server.py

AnthonyHorton · 2018-10-05T14:27:47Z

Thanks for the detailed and quick review! I don't want to push through the merge just yet, though. At the very least want to get tests passing, and ideally remove the overlap and inconsistencies between pocs.tests.test_observatory.py, huntsman.tests.test_observatory.py & huntsman.tests.test_huntsman.py.

AnthonyHorton · 2018-10-05T15:26:09Z

Looking at it a bit more I don't think I actually need anything from test_observatory.py, apart from test_pyro_camera. So I've moved that into test_huntsman.py and removed test_observatory.py. Bingo! Passing tests and a diff that's ~400 lines smaller!

codecov-io · 2018-10-05T15:36:26Z

Codecov Report

Merging #80 into develop will increase coverage by 10.85%.
The diff coverage is 73.98%.

@@             Coverage Diff              @@
##           develop      #80       +/-   ##
============================================
+ Coverage    43.09%   53.94%   +10.85%     
============================================
  Files           29       34        +5     
  Lines         1332     2039      +707     
  Branches       152      220       +68     
============================================
+ Hits           574     1100      +526     
- Misses         743      889      +146     
- Partials        15       50       +35

Impacted Files	Coverage Δ
huntsman/tests/test_huntsman.py	`98.44% <100%> (+0.07%)`	⬆️
huntsman/tests/test_aardvark.py	`100% <100%> (ø)`
huntsman/tests/conftest.py	`63.95% <57.37%> (-16.05%)`	⬇️
huntsman/utils/pyro/__init__.py	`69.87% <69.87%> (ø)`
huntsman/utils/__init__.py	`81.81% <71.42%> (+38.96%)`	⬆️
huntsman/camera/pyro.py	`72.59% <72.59%> (ø)`
huntsman/camera/__init__.py	`73.17% <73.17%> (ø)`
huntsman/tests/test_camera.py	`77.15% <77.15%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d5dc5e...5a0175e. Read the comment docs.

AnthonyHorton · 2018-10-05T16:46:19Z

OK, reduced duplication of code from POCS in huntsman/camera/__init__.py and knocked another ~100 lines off the diff. Can I get a re-review, please, @wtgee? I think this might be OK to merge now.

Haven't done anything for those three unchecked items in the the PR description yet, but I think it's OK for them to be separate Issues & PRs.

wtgee

Great! Looks good. Thanks for a ton of work to get this done in the last few days. Have a good vacation!

wtgee · 2018-10-06T19:17:36Z

huntsman/camera/__init__.py

+
+    # If no camera was specified as primary use the first
+    if primary_camera is None:
+        camera_names = sorted(cameras.keys())


Minor point, but since it's an OrderedDict do you want to skip the sorted and use the first one placed in the dict? I think it comes out the same anyway.

I guess it depends on whether we want to default to the first camera added (which will be Cam00 if there are any local cameras) or to the first camera alphabetically...

wtgee · 2018-10-06T19:24:52Z

huntsman/camera/pyro.py

+            blocking (bool, optional): If False (default) returns immediately after starting
+                the exposure, if True will block until it completes.
+            timeout (u.second, optional): Length of time beyond the length the exposure to wait
+                for exposures to complete. If not given will wait indefinitely.


I'm still on the fence about ever allowing indefinite waits but it's fine for now. Maybe we need an issue for it.

At the least we should be enabling these timeouts in the testing and in the default config files to encourage their use. Will revisit that as part of the general issue of timeouts for all the cameras' threaded actions.

wtgee · 2018-10-06T19:27:22Z

huntsman/camera/pyro.py

+                            'make_plots': make_plots}
+        autofocus_kwargs.update(kwargs)
+
+        focus_dir = os.path.join(os.path.abspath(self.config['directories']['images']), 'focus/')


I assume the trailing / is for rsync?

wtgee · 2018-10-06T19:28:12Z

huntsman/camera/pyro.py

+        """
+        Clean up empty directories left behind by rsysc.
+        """
+        user_at_host, path = source.split(':')


There's some weirdness with the path going on. Maybe an example of what the source input is here.

Ah, yes, the directory cleanup follows the file transfer in the call chain, which just passes on the same source argument that the file transfer uses, which is in user@host:/directory/subdirectory/filename format ready for rysnc.

wtgee · 2018-10-06T19:34:08Z

huntsman/tests/test_camera.py

@@ -0,0 +1,347 @@
+import pytest


Do you want to just name this test_pyro_camera.py and then we can leave it for only Pyro items (if we ever add other camera types just to Huntsman)?

Inclined to leave as is to emphasise that this is a lightly edited copy of pocs/tetsts/test_camera.py, and is supposed to be kept in sync with that. In fact I'll add a comment about that.

wtgee · 2018-10-06T19:38:47Z

huntsman/tests/test_huntsman.py

+    simulator = hardware.get_all_names(without=['camera'])
+    conf['simulator'] = simulator
+    cameras = create_cameras_from_config(conf)
+    obs = Observatory(cameras=cameras,


Note panoptes/POCS#679 and the comments.

I'll have to return to that one. Not sure I understand the underlying Iissue fully.

wtgee · 2018-10-06T19:40:34Z

huntsman/utils/pyro/__init__.py

+    camera servers should be started after the name server, but before POCS.
+
+    Args:
+        ignore_local (bool, optional): If True use the default $POCS/conf_files/pyro_camera.yaml


I don't see this change.

wtgee · 2018-10-06T19:41:57Z

huntsman/utils/pyro/__init__.py

+        print('Starting request loop... (Control-C/Command-C to exit)')
+        try:
+            daemon.requestLoop()
+        finally:


Maybe:

try: daemon... except KeyboardInterrupt: logger.info('Stopping name server') finally: # Cleanup

Not necessary though.

wtgee · 2018-10-06T19:42:48Z

huntsman/utils/pyro/__init__.py

+        print('Starting request loop... (Control-C/Command-C to exit)')
+        try:
+            daemon.requestLoop()
+        finally:


That would only be so that we could add an additional except Exception so we could capture when it fails for some other reason. But again, not necessary for this PR.

wtgee · 2018-10-06T19:43:31Z

scripts/pyro_camera_server.py

+Script to run a Pyro camera server. This should be run on the camera control computer for a
+distributed camera. The configuration for the camera is read from the pyro_camera.yaml/
+pyro_camera_local.yaml config file. The camera servers should be started after the name server,
+but before POCS.


Could we have this script check for a running name server and refuse to start if so? (Can be separate PR)

The run_camera_server function that the script calls does do exactly that. Might be neater to have the script do it instead, though.

Big code dump from panoptes/POCS AstroHuntsman#547

db2d22c

AnthonyHorton added enhancement high priority labels Oct 5, 2018

AnthonyHorton self-assigned this Oct 5, 2018

AnthonyHorton requested review from wtgee and lspitler October 5, 2018 05:33

AnthonyHorton added the camera label Oct 5, 2018

Anthony Horton added 2 commits October 5, 2018 17:33

Fixing imports, paths, removed surplus camera tests

41135eb

Use right load_config, fix camera & aardvark tests

f82464f

wtgee reviewed Oct 5, 2018

View reviewed changes

Anthony Horton added 2 commits October 6, 2018 00:29

Changes from review comments

1f5df58

Move test_pyro_camera into test_huntsman.py, remove test_observatory.py

62f482d

Reduce duplication of code in camera creation

cda7b24

AnthonyHorton changed the title ~~Add support for distributed Cameras - WIP~~ Add support for distributed Cameras Oct 6, 2018

Some missed tidying

72d6873

AnthonyHorton mentioned this pull request Oct 6, 2018

Improve (re)start with distributed cameras #81

Open

3 tasks

wtgee approved these changes Oct 6, 2018

View reviewed changes

wtgee mentioned this pull request Oct 6, 2018

Add support for distributed Cameras - WIP panoptes/POCS#547

Closed

13 tasks

Small changes from re-review

5a0175e

AnthonyHorton merged commit 4862faf into AstroHuntsman:develop Oct 7, 2018

wtgee mentioned this pull request Oct 16, 2018

Add support for distributed Cameras panoptes/POCS#546

Closed

		from pocs.utils import logger as logger_module


		def list_connected_cameras():

Add support for distributed Cameras #80

Add support for distributed Cameras #80

Conversation

AnthonyHorton commented Oct 5, 2018 • edited Loading

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton commented Oct 5, 2018 • edited Loading

AnthonyHorton commented Oct 5, 2018

codecov-io commented Oct 5, 2018 • edited Loading

Codecov Report

AnthonyHorton commented Oct 5, 2018

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyHorton commented Oct 5, 2018 •

edited

Loading

AnthonyHorton Oct 5, 2018 •

edited

Loading

AnthonyHorton Oct 5, 2018 •

edited

Loading

AnthonyHorton Oct 5, 2018 •

edited

Loading

AnthonyHorton commented Oct 5, 2018 •

edited

Loading

codecov-io commented Oct 5, 2018 •

edited

Loading

AnthonyHorton Oct 7, 2018 •

edited

Loading