End of night shutdown improvements. by wtgee · Pull Request #407 · panoptes/POCS

wtgee · 2018-01-22T01:24:24Z

A number of fixes and changes to account for situation when no valid
observations are found but we don't want to shut down the state machine.

If scheduler has a list of observations then it has_valid_observations.
which is True when starting.

Process:

scheduling state calls observatory.get_observation()
get_observation first checks if has_valid_observations is False
if so it will reread the field list.
get_observation either returns an observation or, if none found,
calls clear_available_observations, making has_valid_observations = False.
If no observation is returned to scheduling state, send to parking (then to sleeping).
In sleeping, if has_valid_observations is False then sleep for an hour
then try again (or shut down if run_once is specified).
If has_valid_observations is True and it is safe and dark, shut down because
of unknown error.

Other changes:

Remove reread_fields_file from dispatch scheduler as it is handled at
the observatory level by observatory.get_observation.
Misc small fixes

⚠️ Note: I think I've gotten all the logic correct so that Bad Things™ won't happen. I think. :)

Closes #390

A number of fixes and changes to account for situation when on valid observations are found but we don't want to shut down the state machine. * If scheduler has a list of `observations` then it `has_valid_observations`. which is True when starting. Process: * `scheduling` state calls `observatory.get_observation()` * `get_observation` first checks if `has_valid_observations is False` if so it will reread the field list. * `get_observation` either returns an observation or, if none found, calls `clear_available_observations`, making `has_valid_observations = False`. * If no observation is returned to `scheduling` state, send to `parking` (then to `sleeping`). * In `sleeping`, if `has_valid_observations is False` then sleep for an hour then try again (or shut down if `run_once` is specified). If `has_valid_observations is True` and it is safe and dark, shut down because of unknown error. Other changes: * Remove `reread_fields_file` from dispatch scheduler as it is handled at the `observatory` level by `observatory.get_observation`. * Misc small fixes

codecov · 2018-01-22T01:41:52Z

Codecov Report

Merging #407 into develop will increase coverage by 0.05%.
The diff coverage is 83.63%.

@@            Coverage Diff             @@
##           develop    #407      +/-   ##
==========================================
+ Coverage    69.25%   69.3%   +0.05%     
==========================================
  Files           60      60              
  Lines         4774    4806      +32     
  Branches       659     665       +6     
==========================================
+ Hits          3306    3331      +25     
- Misses        1292    1298       +6     
- Partials       176     177       +1

Impacted Files	Coverage Δ
pocs/scheduler/dispatch.py	`98.03% <100%> (ø)`	⬆️
pocs/observatory.py	`83.66% <100%> (+0.14%)`	⬆️
pocs/core.py	`80.32% <100%> (+0.48%)`	⬆️
pocs/scheduler/scheduler.py	`96.07% <100%> (+1.39%)`	⬆️
pocs/state/states/default/sleeping.py	`62.5% <50%> (-37.5%)`	⬇️
pocs/state/machine.py	`88.27% <66.66%> (+0.08%)`	⬆️
pocs/state/states/default/parked.py	`75% <70.58%> (-25%)`	⬇️

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 2cff654...e943bad. Read the comment docs.

jamessynge

LGTM, besides some nit and questions.

jamessynge · 2018-01-23T00:18:56Z


    # If it is dark and safe we shouldn't be in sleeping state
-    if pocs.is_dark() and pocs.is_safe():
+    if pocs.observatory.scheduler.has_valid_observations and \


pep8 strongly prefers wrapping with parens around expressions rather than using a backslash.

jamessynge · 2018-01-23T00:20:46Z

-    if pocs.is_dark() and pocs.is_safe():
+    if pocs.observatory.scheduler.has_valid_observations and \
+            pocs.is_dark() and pocs.is_safe():
        if pocs.should_retry is False:


Property should_retry is very problematic. It looks like a property, but accessing it modifies its value!

Yes, thanks for calling this out. It's also problematic in that it is never reset without stopping and starting whole system. So if it retries once per night by the third night it is out of retry attempts no matter what. Will fix.

jamessynge · 2018-01-23T00:26:29Z

            'name': 'New Name',
            'position': '20h00m43.7135s +22d42m39.0645s',
-            'priority': 50
+            'priority': 5000


Out of curiosity, why the change in priority?

Thanks for pointing this out too. I'm not sure doing what I did is quite what we want. Changing now.

jamessynge · 2018-01-23T00:27:35Z

        self.logger.debug("Getting observation for observatory")
+
+        # If observation list is empty or a reread is requested
+        if self.scheduler.has_valid_observations is False or \


pep8 strongly prefers wrapping with parens around expressions rather than using a backslash.

Note: This changes some of the state behavior for how parking and retrying is handled. Housekeeping state will now only be called at the very end of an observing run (either in the morning when unsafe or when too many retry attempts). This moves some behavior from `sleeping` into `parked`. * Fix the `should_retry` so it does not modify values * Create a `reset_observing_run` method. Currenlty only resets number of retries * Other PR fixes

* Adding back the ability to reread fields file from the `get_observation` call so that a user can do it manually rather than relying on an empty observations list. But do it cleaner. * Test the rereading of a fields with same entires only with a new higher priority target * Skip duplicate observation entries rather than stopping with assertion error.

it for no reason and is also not testing what it says it should be testing.

wtgee · 2018-01-23T07:24:42Z

I've removed a whole test in test_pocs.py that was hanging. Looking at the test I can't figure out how it was any different from the test below it. Based off of the name of the test, it was designed to be testing some of the interrupt code that a student added about a year ago (via Huntsman) and which has been removed during repository merge and cleanup because it needs some independent work.

Feel weird about just removing a test but I think it is the right course of action.

…s-clean-shutdown-390

Explicitly clear observations for test of no targets

jamessynge · 2018-01-24T00:42:56Z

+                kwargs.get('reread_fields_file', False)):
            self.scheduler.read_field_list()

        self.scheduler.get_observation(*args, **kwargs)


This looks like it should return something. Why not return what get_observation returns?

Good idea to make it explicit, but get_observation actually also sets current_observation, which is then returned. Should maybe think about renaming this.

jamessynge · 2018-01-24T00:46:54Z

                    state_changed = self.goto_next_state()
                except Exception as e:
-                    self.logger.warning("Problem going to next state, exiting loop [{}]".format(e))
+                    self.logger.warning("Problem going from {} to {}, exiting loop [{!r}]".format(


Fine for now, but it would be nice to have a recovery mechanism, so that the system automatically tries to get to mount-parked/dome-closed/state-sleeping if things go bad, and then resumes... or waits until the next noon to restart.

The state_changed is False it will do as you describe, which should be what happens under normal operations. We should probably do as you describe although I would want to do more testing and focus on it. Created #419 to address.

jamessynge · 2018-01-24T00:48:58Z

-    pocs.next_state = 'housekeeping'
+    has_valid_observations = pocs.observatory.scheduler.has_valid_observations
+
+    if (has_valid_observations and pocs.is_safe() is False):


Parentheses not needed here.

jamessynge · 2018-01-24T00:49:08Z

+    if (has_valid_observations and pocs.is_safe() is False):
+        pocs.say("Cleaning up for the night!")
+        pocs.next_state = 'housekeeping'
+    elif (has_valid_observations and pocs.is_safe()):


Excess parens.

jamessynge · 2018-01-24T00:50:34Z

-    pocs.next_state = 'housekeeping'
+    has_valid_observations = pocs.observatory.scheduler.has_valid_observations
+
+    if (has_valid_observations and pocs.is_safe() is False):


Perhaps:

if has_valid_observations: if not pocs.is_safe(): ... else: # pocs.is_safe ... else: # not has_valid_observations ...

* cleaning up code so parking logic is clearer

wtgee requested a review from a team January 22, 2018 01:24

If no messaging then show say commands in log

465f511

jamessynge approved these changes Jan 23, 2018

View reviewed changes

wtgee added 3 commits January 23, 2018 13:35

Better name for private variable

6ff6d44

wtgee mentioned this pull request Jan 23, 2018

Handle meridian flipping better #361

Open

3 tasks

Removing a whole test as it seems to be a duplication of the one below

27a6b45

it for no reason and is also not testing what it says it should be testing.

wtgee added 2 commits January 23, 2018 21:38

Merge branch 'develop' of https://github.com/panoptes/POCS into no-ob…

072c840

…s-clean-shutdown-390

Clear fields_file when clearing observations

420f574

Explicitly clear observations for test of no targets

jamessynge reviewed Jan 24, 2018

View reviewed changes

Feedback from PR

e943bad

* cleaning up code so parking logic is clearer

jamessynge approved these changes Jan 26, 2018

View reviewed changes

jamessynge merged commit e2dd720 into panoptes:develop Jan 26, 2018

wtgee deleted the no-obs-clean-shutdown-390 branch January 26, 2018 21:59

Conversation

wtgee commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jamessynge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgee commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wtgee commented Jan 22, 2018 •

edited

Loading

codecov bot commented Jan 22, 2018 •

edited

Loading