Remove camera creation from Observatory #612

wtgee · 2018-09-20T08:30:59Z

First step toward pulling cameras out of Observatory.
Basic create_cameras_from_config function. This is basically copied over from the _create_cameras that was in Observatory, with no intended behavior change.
Creation removed from Observatory .
Add/remove camera methods.
Updates to observatory init (and testing).
Behavior Change Accessing observatory.primary_camera will now always return a camera if observatory.has_cameras. If no primary camera was set, the first camera in the list will become the primary.

* First step toward pulling cameras out of Observatory. * Basic `create_cameras_from_config` function. * Removed from `Observatory` (not getters/setters yet)

* Adding tests related to adding/removing cameras

* Updates to observatory init (and testing) * **Behavior Change** Accessing `observatory.primary_camera` will now always return a camera if `observatory.has_cameras`.

…a-di

it use `load_config` * Update pocs_shell

jamessynge · 2018-09-20T23:28:09Z

bin/pocs_shell

@@ -152,7 +153,8 @@ class PocsShell(Cmd):
            simulator = []

        try:
-            observatory = Observatory(simulator=simulator)
+            cameras = create_cameras_from_config()
+            observatory = Observatory(simulator=simulator, cameras=cameras)


FWIW, while thinking about adding a supervisor and better messaging, and also while reviewing Anthony's distributed cameras PR (which includes a nameserver), I wondered about introducing some kind of device registry. Something we can discuss in the future.

Yes, I think we will definitely want to change how the Observatory interacts with and discovers devices. Hopefully this gets us closer to doing that.

jamessynge · 2018-09-20T23:29:01Z

pocs/camera/__init__.py

+
+
+def create_cameras_from_config(config=None, logger=None, **kwargs):
+    """Creates a camera object(s)


Creates camera object(s) based on the config.

jamessynge · 2018-09-20T23:29:21Z

pocs/camera/__init__.py

+def create_cameras_from_config(config=None, logger=None, **kwargs):
+    """Creates a camera object(s)
+
+    Loads the cameras via the configuration.


Drop this line.

jamessynge · 2018-09-20T23:30:15Z

pocs/camera/__init__.py

+
+    Note:
+        This does not actually make a connection to the camera. To do so,
+        call 'camera.connect()' explicitly.


... on each camera.

jamessynge · 2018-09-20T23:30:23Z

pocs/camera/__init__.py

+            automatically discover the ports.
+
+    Returns:
+        OrderedDict: An ordered dictionary of created camera objects.


What is the key? And why is it ordered?

Or None if there are no cameras declared in the config.

Added docs to describe. Changed so that an empty OrderedDict is returned instead of None and updated docs.

jamessynge · 2018-09-20T23:36:26Z

pocs/camera/__init__.py

+            # Assign an auto-detected port. If none are left, skip
+            if auto_detect:
+                try:
+                    camera_port = ports.pop()


I'm perplexed by this. If the config file lists two canon cameras with the prefixes of their serial numbers, how do we know the order in which to pop the ports?

This is only if auto_detect is true, which means that it is ignoring the config. Serial number is set when camera object is initialized by lookup (via gphoto2)

Ah. Then I may have misunderstood how to auto_detect and primary work. I included the serial number prefixes and primary:True in pocs_local.yaml on PAN006. My goal is to make sure that a specific camera is treated as primary every time. And ideally we'd get a warning if a specified camera is not found, or if another camera is found.

I think #95 is still true unfortunately. This function could certainly be cleaned up some. Happy to do it here if we want just to make sure it gets done soon but I imagine it will all need to be revisited anyway if we set up separate process controls for the camera. Which hopefully happens soon. :)

jamessynge · 2018-09-20T23:40:58Z

pocs/camera/__init__.py

+    if not config:
+        config = load_config(**kwargs)
+
+    if 'cameras' not in config:


Why is this not after the line: camera_info = kwargs_or_config('cameras')

jamessynge · 2018-09-20T23:41:59Z

pocs/observatory.py

-        self._primary_camera = None
-        self._create_cameras(**kwargs)
+        if not cameras:
+            cameras = OrderedDict()


Why not just raise an exception?

I could. create_dome_from_config is returning None so I'll go for consistency with that (although see comment below).

jamessynge · 2018-09-20T23:43:17Z

pocs/observatory.py

+# Device Getters/Setters
+##########################################################################
+
+    def add_camera(self, cam_name, camera):


Do you intend these two add/remove methods to be used to dynamically add and remove cameras during operation?

Yes, eventually that would be the goal.

jamessynge · 2018-09-20T23:44:20Z

pocs/tests/test_observatory.py

@@ -25,7 +26,11 @@ def simulator():
 @pytest.fixture
 def observatory(config, simulator):
    """Return a valid Observatory instance with a specific config."""
-    obs = Observatory(config=config, simulator=simulator, ignore_local_config=True)
+    cameras = create_cameras_from_config(config)


local=False

Or some such to prevent the pocs_local.yaml from being picked up?

Doesn't the config fixture handle that?

Oh, it may do so. NM.

…a-di

* Fixing docstrings

* Return empty OrderedDict based on config information being present

…a-di

pocs/camera/__init__.py

wtgee · 2018-09-21T22:24:26Z

(I thought I wrote this already but don't see) @jamessynge this is a bit inconsistent with create_dome_from_config and we should standardize. Here I do not require the config parameter and instead pull from our config files if nothing is given. I prefer this but would more prefer all our (eventual) create utils are the same.

Also, as per comments above, we should figure out the behavior if nothing is found. I suppose it makes some sense for the camera to raise an error as it's hard to have an observatory without a camera whereas it's okay to have None for a dome. Still, I think the utils should act the same and we should handle the differences when called. Thoughts?

…a-di

codecov · 2018-09-23T04:03:02Z

Codecov Report

Merging #612 into develop will decrease coverage by 0.99%.
The diff coverage is 61.53%.

@@           Coverage Diff            @@
##           develop     #612   +/-   ##
========================================
- Coverage    67.28%   66.29%   -1%     
========================================
  Files           65       65           
  Lines         5692     5729   +37     
  Branches       798      805    +7     
========================================
- Hits          3830     3798   -32     
- Misses        1653     1718   +65     
- Partials       209      213    +4

Impacted Files	Coverage Δ
pocs/utils/__init__.py	`80.28% <ø> (+0.28%)`	⬆️
pocs/version.py	`100% <100%> (ø)`	⬆️
pocs/camera/__init__.py	`56.98% <56.04%> (-43.02%)`	⬇️
pocs/observatory.py	`75.58% <80%> (-0.82%)`	⬇️
pocs/utils/images/__init__.py	`61.2% <0%> (-12.57%)`	⬇️
pocs/state/states/default/pointing.py	`38.46% <0%> (-5.77%)`	⬇️
pocs/camera/simulator.py	`94.54% <0%> (-5.46%)`	⬇️
pocs/camera/camera.py	`84.73% <0%> (-5.27%)`	⬇️

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 92e7f2c...831c04b. Read the comment docs.

…a-di

AnthonyHorton

Looks good. This new framework will be really useful for refactoring my distributed camera code to allow for camera addition/removal without restarting everything from scratch every time.

AnthonyHorton · 2018-09-26T09:52:12Z

pocs/camera/__init__.py

+
+    Note:
+        This does not actually make a connection to the camera. To do so,
+        call 'camera.connect()' explicitly on each camera.


I don't think this is true, yet. All the existing camera classes (and associated focuser classes) call their own connect() method from their __init__(). I think you know that though, because I didn't spot any calls of camera.connect() in the diff!

On a related note, those calls to camera.connect() should be done in parallel as for some of the supported hardware that triggers an initialisation process that takes quite a while and we can significantly speed up startup by doing all the cameras at the same time.

wtgee · 2018-09-26T10:50:17Z

This new framework will be really useful for refactoring my distributed camera code to allow for camera addition/removal without restarting everything from scratch every time.

I would prefer to merge this before #547 so we can keep some things cleaner, but it will definitely be the bigger change for you. I'll go ahead and get this merged though so that work can start.

…a-di

AnthonyHorton · 2018-09-26T11:16:38Z

I would prefer to merge this before #547 so we can keep some things cleaner, but it will definitely be the bigger change for you. I'll go ahead and get this merged though so that work can start.

Oh, absolutely. What I meant by my comment was just that I'm especially keen to see this merged as soon as possible so that I can use it as a guide for refactoring of #547, which (unlike this PR) is still some way off being ready to merge.

Fix left out of #612.

wtgee added 5 commits September 20, 2018 16:53

Camera dependency injection.

85ffef8

* First step toward pulling cameras out of Observatory. * Basic `create_cameras_from_config` function. * Removed from `Observatory` (not getters/setters yet)

* Removing camera specific tests from observatory

bdd105e

* Adding tests related to adding/removing cameras

* Add/remove camera methods

7c07fc2

* Updates to observatory init (and testing) * **Behavior Change** Accessing `observatory.primary_camera` will now always return a camera if `observatory.has_cameras`.

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

b9a2e1e

…a-di

* config is option for create_cameras_from_config, in which cases

0088074

it use `load_config` * Update pocs_shell

wtgee requested review from jamessynge and AnthonyHorton September 20, 2018 08:31

wtgee added 3 commits September 20, 2018 18:33

Fix old docstring

ea99f15

More docstring fixes

19e81df

Small cleanup

8d5a9ac

jamessynge approved these changes Sep 20, 2018

View reviewed changes

wtgee added 6 commits September 21, 2018 09:46

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

ed5e115

…a-di

* Return an empty OrderedDict if no camera confiration present

e56465a

* Fixing docstrings

Moving list_connected_cameras into cameras file.

0d5efcb

Responding to PR feedback

46d36be

* Return empty OrderedDict based on config information being present

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

df3d577

…a-di

Fix import after moving util function

81cc3b3

jamessynge reviewed Sep 21, 2018

View reviewed changes

pocs/camera/__init__.py Show resolved Hide resolved

wtgee mentioned this pull request Sep 22, 2018

Consistency for hardware creation util functions #619

Closed

wtgee added 2 commits September 22, 2018 12:34

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

4cf2034

…a-di

Cleanup of docstrings

2e8780e

wtgee added the Camera label Sep 22, 2018

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

da3f59c

…a-di

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

56a1797

…a-di

AnthonyHorton approved these changes Sep 26, 2018

View reviewed changes

wtgee added 2 commits September 26, 2018 20:56

Merge branch 'develop' of https://github.com/panoptes/POCS into camer…

d47bcfa

…a-di

Better debug output from camera creation

bc8fca3

Remove old comment

867cb08

AnthonyHorton mentioned this pull request Sep 26, 2018

Add support for distributed Cameras - WIP #547

Closed

13 tasks

wtgee mentioned this pull request Sep 27, 2018

Ensure FLI is not selected as primary camera for solve-field timeout problems AstroHuntsman/huntsman-pocs#67

Closed

Bumping version for camera dependency injection

831c04b

wtgee merged commit aeffd99 into panoptes:develop Sep 27, 2018

wtgee deleted the camera-di branch September 27, 2018 00:41

wtgee mentioned this pull request Sep 30, 2018

Add the cameras to the POCS tests #646

Merged

wtgee added a commit that referenced this pull request Sep 30, 2018

Add the cameras to the POCS tests

b6749a7

Fix left out of #612.

AnthonyHorton pushed a commit to AstroHuntsman/POCS that referenced this pull request Oct 3, 2018

Reorganisation of camera creation in accordance with panoptes#612

2c8d920

AnthonyHorton mentioned this pull request Oct 5, 2018

Add support for distributed Cameras AstroHuntsman/huntsman-pocs#80

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove camera creation from Observatory #612

Remove camera creation from Observatory #612

wtgee commented Sep 20, 2018 •

edited

Loading

jamessynge Sep 20, 2018

wtgee Sep 20, 2018

jamessynge Sep 20, 2018

jamessynge Sep 20, 2018

jamessynge Sep 20, 2018

jamessynge Sep 20, 2018

jamessynge Sep 20, 2018

wtgee Sep 20, 2018

jamessynge Sep 20, 2018

wtgee Sep 20, 2018

jamessynge Sep 21, 2018

wtgee Sep 21, 2018

jamessynge Sep 20, 2018

jamessynge Sep 20, 2018

wtgee Sep 21, 2018

jamessynge Sep 20, 2018

wtgee Sep 20, 2018

jamessynge Sep 20, 2018

wtgee Sep 20, 2018

jamessynge Sep 21, 2018

wtgee commented Sep 21, 2018

codecov bot commented Sep 23, 2018 •

edited

Loading

AnthonyHorton left a comment •

edited

Loading

AnthonyHorton Sep 26, 2018 •

edited

Loading

wtgee commented Sep 26, 2018

AnthonyHorton commented Sep 26, 2018



		def create_cameras_from_config(config=None, logger=None, **kwargs):
		"""Creates a camera object(s)

Remove camera creation from Observatory #612

Remove camera creation from Observatory #612

Conversation

wtgee commented Sep 20, 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

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

wtgee commented Sep 21, 2018

codecov bot commented Sep 23, 2018 • edited Loading

Codecov Report

AnthonyHorton left a comment • edited Loading

Choose a reason for hiding this comment

AnthonyHorton Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

wtgee commented Sep 26, 2018

AnthonyHorton commented Sep 26, 2018

wtgee commented Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 23, 2018 •

edited

Loading

AnthonyHorton left a comment •

edited

Loading

AnthonyHorton Sep 26, 2018 •

edited

Loading