Fix converse and snooze #110

ken-mycroft · 2021-03-24T21:57:26Z

How to use this template

Under each heading below is a short outline of the information required. When submitting a PR, please delete this text and replace it with your own.

The CLA section can be deleted entirely.

Description

Fix outstanding bugs in converse and snooze

If needed follow up with as much detail as required.

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR.
Either delete those that do not apply, or add an x between the square brackets like so: - [x]

[ x] Bugfix
Feature implementation
Refactor of code (without functional changes)
Documentation improvements
Test improvements

Testing

vk tests

Documentation

Does documentation for this already exist? Are there docstrings on all the public methods you added or modified? Have these been updated?

CLA

To protect you, the project, and those who choose to use Mycroft technologies in systems they build, we ask all contributors to sign a Contributor License Agreement.

This agreement clarifies that you are granting a license to the Mycroft Project to freely use your work. Additionally, it establishes that you retain the ownership of your contributed code and intellectual property. As the owner, you are free to use your code in other work, obtain patents, or do anything else you choose with it.

If you haven't already signed the agreement and been added to our public Contributors repo then please head to https://mycroft.ai/cla to initiate the signing process.

krisgesling

Hey Ken, this looks good, just one change I think we should make to keep stop and snooze more distinct.

__init__.py

krisgesling · 2021-04-06T05:43:10Z

__init__.py

+        # valid date like "monday april 5th" LF will return bad data so we make 
+        # sure we never include both. "monday april 5th" becomes "april 5th".
+
+        # I'm sure to get blowback during pr for this ...


krisgesling · 2021-04-06T05:52:36Z

__init__.py

+        # Lingua-franca workaround-001 - day of week and date confuses LF terribly. 
+        # if a day of the week (monday, friday, etc) is included with a
+        # valid date like "monday april 5th" LF will return bad data so we make 
+        # sure we never include both. "monday april 5th" becomes "april 5th".


I've logged this as an issue on that repo. Seems safe enough to do if a valid date exists, though plenty hacky and we should remove it asap.

That parser's being rewritten, because there are all sorts of problems like this. I'd suggest adding a TODO for MycroftAI/lingua-franca#96 which should make this unnecessary, and provide more robust coverage for the same conditions.

Obviously wouldn't be prudent to offer a timetable on that, as there are multiple design choices up in the air in chat, and several higher priorities for everyone on LF. Still, Jarbas has written a lot of the code. He might be due to push, as well.

__init__.py

krisgesling · 2021-04-06T06:02:37Z

__init__.py

+                self.snooze_alarm(message)
+            else:
+                # TODO deal with this
+                self.log.info("AlarmSkill:Converse confused by %s" % (utterances[0],))


this would trigger for any utterance does not contain Stop or Snooze. I don't think we want to do anything here at the moment.

Unless we want to prompt the user - "do you want to turn off the alarm too?"

__init__.py

krisgesling · 2021-04-06T11:59:43Z

__init__.py

+        when_utc = when.astimezone(pytz.utc)
+        self.log.debug("when_utc: %s (this is in utc)" % (when_utc,))
+
+        have_time = False


could this be more descriptive, eg utterance_has_time?

This means we believe we have a time. Its not strictly utterance dependent. It is derived. You may rename it if you like.

krisgesling · 2021-04-06T12:05:44Z

__init__.py

@@ -662,6 +663,117 @@ def _get_alarm_matches(
        # No matches found
        return (status[2], None)

+    def _fallback_get_alarm_matches(self, utt):


Is the intention that this will fully replace _get_alarm_matches() or is it intended to be distinct in some way?

It is used when the normal flow can not find any

…framework

krisgesling · 2021-04-26T07:06:43Z

__init__.py

+                # TODO deal with this
+                self.log.info("AlarmSkill:Converse confused by %s" % (utterances[0],))
+
+            return True  # and consume this phrase


I think we should only consume the phrase if we match on StopBeeping or Snooze - otherwise we are consuming every utterance if an alarm is expired.

This means Mycroft won't respond to anything until you either turn off the alarm or snooze.

This is an interesting point. We are technically in a converse state within an alarm activation. Do we want to open ourselves up to allowing other things to happen before we resolve the current issue with the active alarm? I don't think I know the answer to this so I simply get the code to pass the VK tests which are basically the spec. Now if you would like I suspect you could create a new VK test which initially fails and demonstrates the behavior you would like to alter. I would be open to this.

krisgesling · 2021-04-26T07:07:17Z

__init__.py

-            # Notify all processes to update their loaded configs
-            self.bus.emit(Message("configuration.updated"))
-            del self.settings["user_beep_setting"]
+        #self._restore_listen_beep()


If we're taking this out let's just delete it rather than commenting it out.

__init__.py

krisgesling · 2021-04-26T07:14:49Z

__init__.py


+    def get_tod_matches(self, alarm_time, alarm_list):


A docstring would be helpful with each of these new methods to describe what we're trying to achieve and what we can expect to get returned.

krisgesling · 2021-04-26T07:18:21Z

__init__.py

+    def get_advanced_matches(self, utt, have_time, when, when_utc, user_dow, alarm_list):
+        # holds exact date and time matches
+        exact_matches = []
+        dom = None


All these three letter acronyms make it pretty hard to read, can we rename them to be their longer but more descriptive versions?
Presuming they are:

day_of_month

day_of_week

time_of_day
?

TLAs are all the rage. Your assumptions are indeed correct. If you turn off the fascist linter so every line doesn't have to be split I could use more descriptive names :-)

krisgesling · 2021-04-26T07:28:21Z

__init__.py

+        exact_matches = []
+        dom = None
+        try:
+            dom = self.roman_to_int(utt)


If we're passing the utterance into this method then it's presumably the STT that sometimes returns a number as a roman numeral? But I'm still unsure if you're trying to punk me 🤔

krisgesling · 2021-04-26T12:10:32Z

__init__.py

+        # these alarms match any numbers passed in like
+        # the 25th, 5 etc which we assume are dom
+        # hopefully this won't muss up ordinals
+        dom_matches = []


What happens with an utterance like "cancel my alarm at 5"?

You will get 5 most times. Its just things like the 5th, the 26th which seem to cause the issue.

The number extractors will only extract ordinal numbers when they are passed ordinals=True

(This is to account for fractions. "A third of a second" vs. "for a third time.")

Feels like a number should only be considered a day_of_the_month if it's an ordinal:
"set alarm for 8am on the 5th"
or if a month is included in the utterance but would need to be careful with that:
"set alarm for 8am on June 5"

^^ Does anyone actually say this? "June 5"
Maybe we should stick with ordinals only to be safe. No ones going to say "set 5th alarm for 8am" or "set alarm for 5th hour of the day" < these are the best I could come up with where the ordinal wasn't a day of the month...

Actually I believe ordinal is used to identify an index in a list

That's one use case. It's used to pull numbers with a suffix. ordinals=False will hit "3" but not "3rd"; ordinals=True should do the opposite.

It's imperfect, but in a case like this, I'd do two passes. Time of day will almost never be expressed as an ordinal; as Gez noted, dates almost always will.

krisgesling · 2021-04-26T12:12:57Z

__init__.py

+        for alarm in alarm_list:
+            # get utc time from alarm timestamp
+            dt_obj = datetime.fromtimestamp( alarm['timestamp'] )
+            dt_obj = dt_obj.astimezone(pytz.utc)


Is this the same as mycroft.util.time.to_utc(dt_obj)?

Are you specifically referencing line 779 here? so you want

dt_obj = to_utc(dt_obj)

??

krisgesling · 2021-04-26T12:14:03Z

__init__.py

+            # we also need a local version of our utc time
+            cfg_tz = pytz.timezone(self.local_tz) 
+            dt_local = dt_obj.astimezone(cfg_tz)


Is this the same as mycroft.util.time.now_local()?

no, this is converting the alarm timestamp in the alarm, not now()

krisgesling · 2021-04-26T12:19:07Z

__init__.py

+
+        when_utc = None
+        if when is not None:
+            when_utc = when.astimezone(pytz.utc)


another possible to_utc(when)

I don't really know and its taking a while to chase each of these down so I'll just change them all and rerun the tests and see if anything fails.

I did and they passed so they have been updated.

krisgesling · 2021-04-26T12:54:07Z

__init__.py

+        self.days = [
+                'monday', 
+                'tuesday', 
+                'wednesday', 
+                'thursday', 
+                'friday', 
+                'saturday', 
+                'sunday'
+                ]


Here's a possible approach for localized weekday and month names proposed by one of our community members in the Weather Skill:

from mycroft.util.format import date_time_format ... def initialize(self): date_time_format.cache(self.lang) if self.lang in date_time_format.lang_config.keys(): self.weekdays = list(date_time_format.lang_config[self.lang]['weekday'].values()) self.months = list(date_time_format.lang_config[self.lang]['month'].values())

Thanks Gez, seems to work well.

Possible TODO for simplification when MycroftAI/lingua-franca#185 is merged and released (which is currently slated for LF 0.5.0)

That PR doesn't currently expose formatting information via the config interface, because those values live in different resource files. However, it could be rolled into the same accessor, or a separate one for the purpose.

I just reckon something like list(lingua_franca.config.get('weekdays', self.lang)) would be less painful =P

Obviously, you shouldn't have to work around the parser at all, but the config PR will almost certainly beat the datetime rewrite to the presses.

__init__.py

krisgesling · 2021-04-28T02:11:43Z

test/behave/alarms.py

@@ -0,0 +1,129 @@
+import time
+


Looks like this file got dropped in the wrong place and should be overwriting test/behave/steps/alarms.py?

krisgesling · 2021-04-28T02:25:48Z

test/behave/steps/alarms.py

@@ -10,6 +10,7 @@
 @given('an alarm is set for {alarm_time}')
 def given_set_alarm(context, alarm_time):
    emit_utterance(context.bus, 'set an alarm for {}'.format(alarm_time))
+    time.sleep(3)


wait_for_dialog() has an optional timeout arg, so you could set this to a higher value instead

krisgesling · 2021-04-28T02:28:13Z

test/behave/skill-alarm.feature

@@ -494,3 +448,40 @@ Feature: Alarm skill functionality
    | remove all alarms |
    | remove every alarm |
    | delete every alarm |
+
+# Jira MS-72 https://mycroft.atlassian.net/browse/MS-72
+  Scenario Outline: user snoozes a beeping alarm


These significantly increase the time it takes for these tests to run. So before this gets merged into the marketplace we really need to think through which tests are going to run when, and how we define that.

Yes, I considered adding configurable ability for snooze so we could drop it to like 1 minute during testing but there are only so many hours in a day. I am not adverse to removing the snooze tests or any other tests causing excessive execution time but losing tests does not feel right.

krisgesling · 2021-04-28T02:36:48Z

test/behave/skill-alarm.feature

+    | set alarm in 5 minutes |
+    | new alarm in 5 minutes and 30 seconds |


For utterances without a verb, I think we need to either:

create an alarm

catch the utterance and disambiguate

Currently the Alarm Skill won't trigger from this utterance and so you would either get the Date Time Skill or a fallback even though we have the keyword alarm and a time / duration.

My assumption is that this should be a Padatious intent so it only matches specific short utterances and we don't have any utterance with the word "alarm" triggering this Skill.
eg

alarm alarm in {duration} alarm at {time}

So, where to start. First, these tests all pass so the behavior Derick requested was achieved. The utterance 'alarm 6' will now create an alarm at 6. Second, indeed, this triggers for nearly anything which contains the word 'alarm' in it. I don't know if that's a good or bad thing as we are reaching the point of 'the little boy and the dyke' mode where you catch something new here and lose something old there. This will get worse before it gets better with our current design which is why I push for new requirements to be specified using VK tests. This will be the only hope we have of catching intent conundrums as we move forward. How that squares with reducing test coverage by removing tests to decrease test execution times is left as an exercise for the reader.

krisgesling · 2021-04-28T02:37:47Z

test/behave/alarms.py

+@then('"mycroft-alarm" should stop beeping and start beeping again in 10 minutes')
+def then_stop_and_start_beeping(context):


We could merge the 5 and 10 minute variations of this step by extracting the period as a variable:

@then('"mycroft-alarm" should stop beeping and start beeping again in {snooze_duration}') def then_stop_and_start_beeping(context, snooze_duration):

krisgesling · 2021-04-28T02:38:14Z

test/behave/alarms.py

+
+@then('"mycroft-alarm" should stop beeping')
+def then_stop_beeping(context):
+    # TODO Implement


Could we add a message that would be emitted when the beep is stopped in code eg mycroft.alarm.silenced? Is that worthwhile even though we can't definitely say "there is no beeping sound being output"

We should also call it from the methods below.

Well I actually added just this to indicate beeping has started in the skill proper. I am not sure why you would need to know when the beeping stopped as well (those methods below I believe rely on the new message I already added for beeping started if I am not mistaken), however, this is something that would probably need to go in the audio service as we have no idea when beeping stops and they are variable length.

krisgesling · 2021-04-28T02:46:07Z

__init__.py

+        # these alarms match any numbers passed in like
+        # the 25th, 5 etc which we assume are dom
+        # hopefully this won't muss up ordinals
+        dom_matches = []


Feels like a number should only be considered a day_of_the_month if it's an ordinal:
"set alarm for 8am on the 5th"
or if a month is included in the utterance but would need to be careful with that:
"set alarm for 8am on June 5"

^^ Does anyone actually say this? "June 5"
Maybe we should stick with ordinals only to be safe. No ones going to say "set 5th alarm for 8am" or "set alarm for 5th hour of the day" < these are the best I could come up with where the ordinal wasn't a day of the month...

krisgesling · 2021-04-28T02:47:07Z

__init__.py

+
+    def get_dow_from_utterance(self, utt):
+        user_dow = None
+        result = re.findall('(mon|tues|wed|thurs|fri|sat|sun)day', utt)


can we use self.weekdays here instead so it's localized?

I'm gonna leave the last two to you. I believe ordinals are indices not days of the month as I mentioned in a previous comment and the above snippet has nuances regarding the minimized day of week name (notice they are not monday, tuesday, etc) and how the data arrives so please feel free to make any stylistic changes you would like.

ken-mycroft added 2 commits March 24, 2021 15:49

fix basic converse bug and fix basic snooze functionality

da6213e

forgot vocab fix

2e4cdf6

ken-mycroft requested a review from krisgesling March 24, 2021 21:57

krisgesling reviewed Mar 26, 2021

View reviewed changes

__init__.py Outdated Show resolved Hide resolved

ken-mycroft added 3 commits March 31, 2021 12:07

hoping the absence of a Snooze.voc file does not crash non US users

38abc25

this works better but will give me grief during a PR review

4114550

quick fix based on Gez input

a0e1ee8

krisgesling reviewed Apr 6, 2021

View reviewed changes

refactor and clean up. may also require changes I made to the vktest …

498bd86

…framework

krisgesling reviewed Apr 26, 2021

View reviewed changes

ken-mycroft added 3 commits April 26, 2021 16:59

corrections per PR and add back in snooze tests

67b71d2

fix bug where you cant exit a multi select exchange

5256864

fix broken snooze vk tests with 10 minute period

5994995

krisgesling reviewed Apr 28, 2021

View reviewed changes

ken-mycroft and others added 2 commits April 28, 2021 11:42

remove file from incorrect location

3fa1bde

Add #todo for consistent cancel experience

e43221b

		\| set alarm in 5 minutes \|
		\| new alarm in 5 minutes and 30 seconds \|

		@then('"mycroft-alarm" should stop beeping and start beeping again in 10 minutes')
		def then_stop_and_start_beeping(context):

Fix converse and snooze #110

Are you sure you want to change the base?

Fix converse and snooze #110

Conversation

ken-mycroft commented Mar 24, 2021

How to use this template

Description

Type of PR

Testing

Documentation

CLA

krisgesling 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

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

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

Choose a reason for hiding this comment