Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Improve the speed of waiting for dialogs helper function #2946

Merged
merged 6 commits into from
Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions test/integrationtests/voight_kampff/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#

from .tools import (emit_utterance, wait_for_dialog, then_wait,
then_wait_fail, mycroft_responses,
print_mycroft_responses, wait_for_audio_service)
"""Public API into the voight_kampff package."""
from .tools import (
emit_utterance,
format_dialog_match_error,
mycroft_responses,
print_mycroft_responses,
then_wait,
then_wait_fail,
wait_for_audio_service,
wait_for_dialog,
wait_for_dialog_match,
)
95 changes: 79 additions & 16 deletions test/integrationtests/voight_kampff/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#

"""Common tools to use when creating step files for behave tests."""

import time

from mycroft.audio.utils import wait_while_speaking
from mycroft.messagebus import Message


TIMEOUT = 10
DEFAULT_TIMEOUT = 10


def then_wait(msg_type, criteria_func, context, timeout=None):
Expand Down Expand Up @@ -67,7 +67,7 @@ def then_wait_fail(msg_type, criteria_func, context, timeout=None):
tuple (bool, str) test status and debug output
"""
status, debug = then_wait(msg_type, criteria_func, context, timeout)
return (not status, debug)
return not status, debug


def mycroft_responses(context):
Expand Down Expand Up @@ -95,15 +95,50 @@ def print_mycroft_responses(context):
print(mycroft_responses(context))


def emit_utterance(bus, utt):
"""Emit an utterance on the bus.
def format_dialog_match_error(potential_matches, speak_messages):
"""Format error message to be displayed when an expected

This is similar to the mycroft_responses function above. The difference
is that here the expected responses are passed in instead of making
a second loop through message bus messages.

Args:
potential_matches (list): one of the dialog files in this list were
expected to be spoken
speak_messages (list): "speak" event messages from the message bus
that don't match the list of potential matches.

Returns: (str) Message detailing the error to the user
"""
error_message = (
'Expected Mycroft to respond with one of:\n'
f"\t{', '.join(potential_matches)}\n"
"Actual response(s):\n"
)
if speak_messages:
for message in speak_messages:
meta = message.data.get("meta")
if meta is not None:
if 'dialog' in meta:
error_message += f"\tDialog: {meta['dialog']}"
if 'skill' in meta:
error_message += f" (from {meta['skill']} skill)\n"
error_message += f"\t\tUtterance: {message.data['utterance']}\n"
else:
error_message += "\tMycroft didn't respond"

return error_message


def emit_utterance(bus, utterance):
"""Emit an utterance event on the message bus.

Args:
bus (InterceptAllBusClient): Bus instance to listen on
dialogs (list): list of acceptable dialogs
utterance (str): list of acceptable dialogs
"""
bus.emit(Message('recognizer_loop:utterance',
data={'utterances': [utt],
data={'utterances': [utterance],
'lang': 'en-us',
'session': '',
'ident': time.time()},
Expand All @@ -121,18 +156,46 @@ def wait_for_dialog(bus, dialogs, context=None, timeout=None):
provided by context or 10 seconds
"""
if context:
timeout = timeout or context.step_timeout
timeout_duration = timeout or context.step_timeout
else:
timeout = timeout or TIMEOUT
start_time = time.monotonic()
while time.monotonic() < start_time + timeout:
timeout_duration = timeout or DEFAULT_TIMEOUT
wait_for_dialog_match(bus, dialogs, timeout_duration)


def wait_for_dialog_match(bus, dialogs, timeout=DEFAULT_TIMEOUT):
"""Match dialogs spoken to the specified list of expected dialogs.

Only one of the dialogs in the provided list need to match for this
check to be successful.

Args:
bus (InterceptAllBusClient): Bus instance to listen on
dialogs (list): list of acceptable dialogs
timeout (int): how long to wait for the message, defaults to timeout
provided by context or 10 seconds

Returns:
A boolean indicating if a match was found and the list of "speak"
events found on the message bus during the matching process.
"""
match_found = False
speak_messages = list()
timeout_time = time.monotonic() + timeout
while time.monotonic() < timeout_time:
for message in bus.get_messages('speak'):
speak_messages.append(message)
dialog = message.data.get('meta', {}).get('dialog')
print('dialog: ', dialog, '\tutterance: ', message.data['utterance'])
chrisveilleux marked this conversation as resolved.
Show resolved Hide resolved
if dialog in dialogs:
bus.clear_messages()
return
bus.new_message_available.wait(0.5)
bus.clear_messages()
wait_while_speaking()
match_found = True
break
bus.clear_messages()
forslund marked this conversation as resolved.
Show resolved Hide resolved
if match_found:
break
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can bus.new_message_available.wait(0.5) be used to make it a bit more snappy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, using the message_available event caused a very tight loop because there are so many message bus messages being emitted. Instead of looping ten times, as this code does, it looped hundreds of times. I would like to avoid these types of waits completely and just use message bus events with timeouts.

Copy link
Collaborator

@forslund forslund Jul 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing the issue with the tight loop, this waits for messages as soon as messages arrive we want to evaluate them. But you are right in this case an event driven approach would be much tidier. Can we change the implementation to something similar to:

def wait_for_dialog_match(bus, dialogs, timeout=DEFAULT_TIMEOUT):
    """Match dialogs spoken to the specified list of expected dialogs.
    Only one of the dialogs in the provided list need to match for this
    check to be successful.
    Args:
        bus (InterceptAllBusClient): Bus instance to listen on
        dialogs (list): list of acceptable dialogs
        timeout (int): how long to wait for the message, defaults to timeout
                       provided by context or 10 seconds
    Returns:
        A boolean indicating if a match was found and the list of "speak"
        events found on the message bus during the matching process.
    """
    match_found = Event()
    speak_messages = list()

    def dialog_handler(message):
        nonlocal match_found
        nonlocal speak_messages
        dialog = message.data.get('meta', {}).get('dialog')
        speak_messages.append(message)
        if dialog in dialogs:
            wait_while_speaking()
            match_found.set()

    bus.on('speak', dialog_handler)
    # Check if the dialog has already been received
    historical_result = check_historical_messages()
    if historical_result:
        match_found.set()
    else:
        match_found.wait(timeout=timeout)
        bus.remove('speak', dialog_handler)
    return match_found.is_set(), speak_messages

Where historical messages would be just check the current stack of received messages for matches to handle the case where the desired 'speak' message arrives between the when and then steps.


return match_found, speak_messages


def wait_for_audio_service(context, message_type):
Expand All @@ -148,7 +211,7 @@ def wait_for_audio_service(context, message_type):
msg_type = 'mycroft.audio.service.{}'.format(message_type)

def check_for_msg(message):
return (message.msg_type == msg_type, '')
return message.msg_type == msg_type, ''

passed, debug = then_wait(msg_type, check_for_msg, context)

Expand Down