From 9142bd4cbabaa402b52ed6841951360ff59101f6 Mon Sep 17 00:00:00 2001 From: Kathrin Bujna Date: Tue, 2 Nov 2021 16:32:32 +0100 Subject: [PATCH] markers / validation (#10012) * validation; name vs tag; specify expected number of sub-markers;... * Delete old Marker test fixtures and associated files. * Fix tests after rebasing main * First pass on domain validation * Add some tests around domain validation Need to work out why set union over a generator expression wasn't working * Remove unused markers test data * Config validation tests * Fix lint * add tests for from_path; simplify other tests * make domain path optional for _run_markers * Codeclimate fixes * Codeclimate fixes * Swap over warnings for errors Co-authored-by: aeshky Co-authored-by: Matthew Summers --- data/test_markers/config_dir/config_1.yml | 5 - data/test_markers/config_dir/config_2.yml | 7 - data/test_markers/config_dir/config_3.yml | 10 - data/test_markers/config_dir/not_a_config.txt | 1 - data/test_markers/config_invalid.yml | 7 - data/test_markers/config_operators.yml | 40 -- data/test_markers/config_simple.yml | 10 - data/test_markers/extracted_markers.json | 161 ------ rasa/cli/evaluate.py | 15 +- rasa/core/evaluation/marker.py | 131 +++-- rasa/core/evaluation/marker_base.py | 506 +++++++++++------- tests/cli/test_rasa_evaluate_markers.py | 9 +- tests/conftest.py | 30 -- tests/core/evaluation/test_marker.py | 386 ++++++++----- 14 files changed, 676 insertions(+), 642 deletions(-) delete mode 100644 data/test_markers/config_dir/config_1.yml delete mode 100644 data/test_markers/config_dir/config_2.yml delete mode 100644 data/test_markers/config_dir/config_3.yml delete mode 100644 data/test_markers/config_dir/not_a_config.txt delete mode 100644 data/test_markers/config_invalid.yml delete mode 100644 data/test_markers/config_operators.yml delete mode 100644 data/test_markers/config_simple.yml delete mode 100644 data/test_markers/extracted_markers.json diff --git a/data/test_markers/config_dir/config_1.yml b/data/test_markers/config_dir/config_1.yml deleted file mode 100644 index 7ac303a96767..000000000000 --- a/data/test_markers/config_dir/config_1.yml +++ /dev/null @@ -1,5 +0,0 @@ -markers: - - marker: no_restart - condition: - - action_not_executed: - - action_restart diff --git a/data/test_markers/config_dir/config_2.yml b/data/test_markers/config_dir/config_2.yml deleted file mode 100644 index c38c0e38ec2a..000000000000 --- a/data/test_markers/config_dir/config_2.yml +++ /dev/null @@ -1,7 +0,0 @@ -markers: - - marker: all_required_data_gathered - condition: - - slot_is_set: - - travel_flight_class - - travel_departure - - travel_destination diff --git a/data/test_markers/config_dir/config_3.yml b/data/test_markers/config_dir/config_3.yml deleted file mode 100644 index c684c641b843..000000000000 --- a/data/test_markers/config_dir/config_3.yml +++ /dev/null @@ -1,10 +0,0 @@ -markers: - - marker: carbon_offset_calculated - operator: AND - condition: - - slot_is_set: - - travel_flight_class - - travel_departure - - travel_destination - - action_executed: - - provide_carbon_estimate diff --git a/data/test_markers/config_dir/not_a_config.txt b/data/test_markers/config_dir/not_a_config.txt deleted file mode 100644 index ed8beabea91b..000000000000 --- a/data/test_markers/config_dir/not_a_config.txt +++ /dev/null @@ -1 +0,0 @@ -some other file that should not be processed by the config reader. \ No newline at end of file diff --git a/data/test_markers/config_invalid.yml b/data/test_markers/config_invalid.yml deleted file mode 100644 index 5a1de1e71015..000000000000 --- a/data/test_markers/config_invalid.yml +++ /dev/null @@ -1,7 +0,0 @@ -markers: -- marker: incorrectly_indented_yaml - condition: - - slot_1 - - slot_2 - - action_executed: - - action_1 diff --git a/data/test_markers/config_operators.yml b/data/test_markers/config_operators.yml deleted file mode 100644 index b3f66e7f8380..000000000000 --- a/data/test_markers/config_operators.yml +++ /dev/null @@ -1,40 +0,0 @@ -markers: - - marker: no_restart - condition: - - action_not_executed: - - action_restart - - - marker: all_required_data_gathered - condition: - - slot_is_set: - - travel_flight_class - - travel_departure - - travel_destination - - - marker: carbon_offset_calculated - operator: AND - condition: - - slot_is_set: - - travel_flight_class - - travel_departure - - travel_destination - - action_executed: - - provide_carbon_estimate - - - marker: chitchat_or_FAQ - operator: OR - condition: - - action_executed: - - utter_explain_why_offset_travel - - action_executed: - - utter_faq - - - marker: dialog_followed_expected_order - operator: SEQ - condition: - - action_executed: - - requested_slot_travel_flight_class - - action_executed: - - requested_slot_travel_departure - - action_executed: - - requested_slot_travel_destination diff --git a/data/test_markers/config_simple.yml b/data/test_markers/config_simple.yml deleted file mode 100644 index c684c641b843..000000000000 --- a/data/test_markers/config_simple.yml +++ /dev/null @@ -1,10 +0,0 @@ -markers: - - marker: carbon_offset_calculated - operator: AND - condition: - - slot_is_set: - - travel_flight_class - - travel_departure - - travel_destination - - action_executed: - - provide_carbon_estimate diff --git a/data/test_markers/extracted_markers.json b/data/test_markers/extracted_markers.json deleted file mode 100644 index a6fdfff4f1ae..000000000000 --- a/data/test_markers/extracted_markers.json +++ /dev/null @@ -1,161 +0,0 @@ -[ - { - "tracker_ID": "dialogue_1", - "bot-version": "", - "markers": [ - { - "marker": "no_restart", - "num_preceding_user_turns": [ - 20 - ], - "timestamps": [ - 63123.21442 - ] - }, - { - "marker": "all_required_data_gathered", - "num_preceding_user_turns": [ - 8 - ], - "timestamps": [ - 24123.21452 - ] - }, - { - "marker": "carbon_offset_calculated", - "num_preceding_user_turns": [ - 8, - 12 - ], - "timestamps": [ - 24723.21485, - 33100.24350 - ] - }, - { - "marker": "chitchat_and_FAQ", - "num_preceding_user_turns": [ - 2, - 9, - 13 - ], - "timestamps": [ - 14123.21452, - 25163.21452, - 35173.21452 - ] - }, - { - "marker": "dialog_expected_order", - "num_preceding_user_turns": [ - 5 - ], - "timestamps": [ - 20123.54555 - ] - } - ] - }, - { - "tracker_ID": "dialogue_2", - "bot-version": "", - "markers": [ - { - "marker": "no_restart", - "num_preceding_user_turns": [ - 13 - ], - "timestamps": [ - 63123.21442 - ] - }, - { - "marker": "all_required_data_gathered", - "num_preceding_user_turns": [ - 8 - ], - "timestamps": [ - 24123.21452 - ] - }, - { - "marker": "carbon_offset_calculated", - "num_preceding_user_turns": [ - 8, - 12 - ], - "timestamps": [ - 24723.21485, - 33100.24350 - ] - }, - { - "marker": "chitchat_and_FAQ", - "num_preceding_user_turns": [], - "timestamps": [] - }, - { - "marker": "dialog_expected_order", - "num_preceding_user_turns": [ - 7 - ], - "timestamps": [ - 20123.54555 - ] - } - ] - }, - { - "tracker_ID": "dialogue_3", - "bot-version": "", - "markers": [ - { - "marker": "no_restart", - "num_preceding_user_turns": [], - "timestamps": [] - }, - { - "marker": "all_required_data_gathered", - "num_preceding_user_turns": [ - 8, - 20 - ], - "timestamps": [ - 24123.21452, - 25123.21452 - ] - }, - { - "marker": "carbon_offset_calculated", - "num_preceding_user_turns": [ - 10, - 22 - ], - "timestamps": [ - 24723.21485, - 33100.24350 - ] - }, - { - "marker": "chitchat_and_FAQ", - "num_preceding_user_turns": [ - 2 - ], - "timestamps": [ - 14123.21452 - ] - }, - { - "marker": "dialog_expected_order", - "num_preceding_user_turns": [ - 9, - 19 - ], - "timestamps": [ - 20123.54555, - 20123.54555 - ] - } - ] - } -] diff --git a/rasa/cli/evaluate.py b/rasa/cli/evaluate.py index a765d202af40..f09e4c7a95c7 100644 --- a/rasa/cli/evaluate.py +++ b/rasa/cli/evaluate.py @@ -1,4 +1,5 @@ import argparse +from rasa.shared.core.domain import Domain from typing import List, Text, Optional from rasa.core.utils import AvailableEndpoints @@ -87,6 +88,7 @@ def _run_markers_cli(args: argparse.Namespace) -> None: seed, count, args.endpoints, + args.domain, args.strategy, args.config, args.output_filename, @@ -98,6 +100,7 @@ def _run_markers( seed: Optional[int], count: Optional[int], endpoint_config: Text, + domain_path: Optional[Text], strategy: Text, config: Text, output_filename: Text, @@ -112,6 +115,8 @@ def _run_markers( except 'all'). endpoint_config: Path to the endpoint configuration defining the tracker store to use. + domain_path: Path to the domain specification to use when validating the + marker definitions. strategy: Strategy to use when selecting trackers to extract from. config: Path to the markers definition file to use. output_filename: Path to write out the extracted markers. @@ -128,8 +133,16 @@ def _run_markers( "A file with the stats filename already exists" ) - tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed) + domain = Domain.load(domain_path) if domain_path else None markers = Marker.from_path(config) + + if domain and not markers.validate_against_domain(domain): + rasa.shared.utils.cli.print_error_and_exit( + "Validation errors were found in the markers definition. " + "Please see errors listed above and fix before running again." + ) + + tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed) markers.export_markers(tracker_loader.load(), output_filename, stats_file) diff --git a/rasa/core/evaluation/marker.py b/rasa/core/evaluation/marker.py index 95eba95c8d99..9dec774fe316 100644 --- a/rasa/core/evaluation/marker.py +++ b/rasa/core/evaluation/marker.py @@ -1,20 +1,22 @@ +from rasa.shared.core.domain import Domain from typing import Optional, Text, List from rasa.core.evaluation.marker_base import ( - CompoundMarker, - AtomicMarker, + OperatorMarker, + ConditionMarker, MarkerRegistry, - Marker, - InvalidMarkerConfig, ) from rasa.shared.core.events import ActionExecuted, SlotSet, UserUttered, Event +import logging + +logger = logging.getLogger(__name__) @MarkerRegistry.configurable_marker -class AndMarker(CompoundMarker): +class AndMarker(OperatorMarker): """Checks that all sub-markers apply.""" @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "and" @@ -28,25 +30,38 @@ def _non_negated_version_applies_at(self, event: Event) -> bool: @MarkerRegistry.configurable_marker -class OrMarker(CompoundMarker): +class OrMarker(OperatorMarker): """Checks that at least one sub-marker applies.""" @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "or" + def _non_negated_version_applies_at(self, event: Event) -> bool: + return any(marker.history[-1] for marker in self.sub_markers) + + +@MarkerRegistry.configurable_marker +class NotMarker(OperatorMarker): + """Checks that at least one sub-marker applies.""" + @staticmethod - def negated_tag() -> Optional[Text]: - """Returns the tag to be used in a config file for the negated version.""" + def positive_tag() -> Text: + """Returns the tag to be used in a config file.""" return "not" + @staticmethod + def expected_number_of_sub_markers() -> Optional[int]: + """Returns the expected number of sub-markers (if there is any).""" + return 1 + def _non_negated_version_applies_at(self, event: Event) -> bool: - return any(marker.history[-1] for marker in self.sub_markers) + return not self.sub_markers[0].history[-1] @MarkerRegistry.configurable_marker -class SequenceMarker(CompoundMarker): +class SequenceMarker(OperatorMarker): """Checks that all sub-markers apply consecutively in the specified order. Given a sequence of sub-markers `m_0, m_1,...,m_n`, the sequence marker applies @@ -55,10 +70,15 @@ class SequenceMarker(CompoundMarker): """ @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "seq" + @staticmethod + def negated_tag() -> Text: + """Returns the tag to be used in a config file.""" + return "not(seq)" + def _to_str_with(self, tag: Text) -> Text: sub_markers_str = " -> ".join(str(marker) for marker in self.sub_markers) return f"{tag}({sub_markers_str})" @@ -77,27 +97,15 @@ def _non_negated_version_applies_at(self, event: Event) -> bool: @MarkerRegistry.configurable_marker -class OccurrenceMarker(CompoundMarker): +class OccurrenceMarker(OperatorMarker): """Checks that all sub-markers applied at least once in history. It doesn't matter if the sub markers stop applying later in history. If they applied at least once they will always evaluate to `True`. """ - def __init__( - self, markers: List[Marker], negated: bool = False, name: Optional[Text] = None - ) -> None: - """Creates marker (see parent class for full docstring).""" - super().__init__(markers, negated, name) - if len(markers) != 1: - own_tag = self.negated_tag() if negated else self.tag() - raise InvalidMarkerConfig( - f"It is not possible to have multiple markers below a '{own_tag}' " - f"marker." - ) - @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "at_least_once" @@ -106,8 +114,20 @@ def negated_tag() -> Optional[Text]: """Returns the tag to be used in a config file for the negated version.""" return "never" + @staticmethod + def expected_number_of_sub_markers() -> Optional[int]: + """Returns the expected number of sub-markers (if there is any).""" + return 1 + def _non_negated_version_applies_at(self, event: Event) -> bool: - return any(any(marker.history) for marker in self.sub_markers) + occurred_before = False + + if self.history: + occurred_before = self.history[-1] + if self.negated: + occurred_before = not occurred_before + + return occurred_before or self.sub_markers[0].history[-1] def relevant_events(self) -> List[int]: """Only return index of first match (see parent class for full docstring).""" @@ -118,11 +138,11 @@ def relevant_events(self) -> List[int]: @MarkerRegistry.configurable_marker -class ActionExecutedMarker(AtomicMarker): +class ActionExecutedMarker(ConditionMarker): """Checks whether an action is executed at the current step.""" @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "action_executed" @@ -131,12 +151,27 @@ def negated_tag() -> Optional[Text]: """Returns the tag to be used in a config file for the negated version.""" return "action_not_executed" + def validate_against_domain(self, domain: Domain) -> bool: + """Checks that this marker (and its children) refer to entries in the domain. + + Args: + domain: The domain to check against + """ + valid = self.text in domain.action_names_or_texts + + if not valid: + logger.error( + f"Referenced action '{self.text}' does not exist in the domain" + ) + + return valid + def _non_negated_version_applies_at(self, event: Event) -> bool: return isinstance(event, ActionExecuted) and event.action_name == self.text @MarkerRegistry.configurable_marker -class IntentDetectedMarker(AtomicMarker): +class IntentDetectedMarker(ConditionMarker): """Checks whether an intent is expressed at the current step. More precisely it applies at an event if this event is a `UserUttered` event @@ -145,7 +180,7 @@ class IntentDetectedMarker(AtomicMarker): """ @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "intent_detected" @@ -154,6 +189,21 @@ def negated_tag() -> Optional[Text]: """Returns the tag to be used in a config file for the negated version.""" return "intent_not_detected" + def validate_against_domain(self, domain: Domain) -> bool: + """Checks that this marker (and its children) refer to entries in the domain. + + Args: + domain: The domain to check against + """ + valid = self.text in domain.intent_properties + + if not valid: + logger.error( + f"Referenced intent '{self.text}' does not exist in the domain" + ) + + return valid + def _non_negated_version_applies_at(self, event: Event) -> bool: return isinstance(event, UserUttered) and self.text in [ event.intent_name, @@ -162,14 +212,14 @@ def _non_negated_version_applies_at(self, event: Event) -> bool: @MarkerRegistry.configurable_marker -class SlotSetMarker(AtomicMarker): +class SlotSetMarker(ConditionMarker): """Checks whether a slot is set at the current step. The actual `SlotSet` event might have happened at an earlier step. """ @staticmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" return "slot_is_set" @@ -178,6 +228,19 @@ def negated_tag() -> Optional[Text]: """Returns the tag to be used in a config file for the negated version.""" return "slot_is_not_set" + def validate_against_domain(self, domain: Domain) -> bool: + """Checks that this marker (and its children) refer to entries in the domain. + + Args: + domain: The domain to check against. + """ + valid = any(self.text == slot.name for slot in domain.slots) + + if not valid: + logger.error(f"Referenced slot '{self.text}' does not exist in the domain") + + return valid + def _non_negated_version_applies_at(self, event: Event) -> bool: if isinstance(event, SlotSet) and event.key == self.text: # slot is set if and only if it's value is not `None` diff --git a/rasa/core/evaluation/marker_base.py b/rasa/core/evaluation/marker_base.py index 72986624f3ae..f051344b93d9 100644 --- a/rasa/core/evaluation/marker_base.py +++ b/rasa/core/evaluation/marker_base.py @@ -1,6 +1,7 @@ from __future__ import annotations import os from abc import ABC, abstractmethod +from rasa.shared.core.domain import Domain from rasa.shared.core.trackers import DialogueStateTracker from rasa.utils.io import WriteRow from typing import ( @@ -14,6 +15,7 @@ Type, TypeVar, Union, + Any, ) from pathlib import Path @@ -39,8 +41,8 @@ class MarkerRegistry: """Keeps track of tags that can be used to configure markers.""" all_tags: Set[Text] = set() - condition_tag_to_marker_class: Dict[Text, Type[AtomicMarker]] = {} - operator_tag_to_marker_class: Dict[Text, Type[CompoundMarker]] = {} + condition_tag_to_marker_class: Dict[Text, Type[ConditionMarker]] = {} + operator_tag_to_marker_class: Dict[Text, Type[OperatorMarker]] = {} marker_class_to_tag: Dict[Type[Marker], Text] = {} negated_tag_to_tag: Dict[Text, Text] = {} tag_to_negated_tag: Dict[Text, Text] = {} @@ -62,7 +64,7 @@ def configurable_marker(cls, marker_class: Type[Marker]) -> Type[Marker]: if not issubclass(marker_class, Marker): raise RuntimeError("Can only register marker classes as configurable.") - tag = marker_class.tag() + tag = marker_class.positive_tag() negated_tag = marker_class.negated_tag() cls._register_tags(tags={tag, negated_tag}) cls._register_tag_class(marker_class=marker_class, positive_tag=tag) @@ -108,10 +110,15 @@ def get_non_negated_tag(cls, tag_or_negated_tag: Text) -> Tuple[Text, bool]: def _register_tag_class( cls, marker_class: Type[Marker], positive_tag: Text ) -> None: - if issubclass(marker_class, AtomicMarker): + if issubclass(marker_class, ConditionMarker): cls.condition_tag_to_marker_class[positive_tag] = marker_class - else: + elif issubclass(marker_class, OperatorMarker): cls.operator_tag_to_marker_class[positive_tag] = marker_class + else: + raise RuntimeError( + f"Can only register `{OperatorMarker.__name__} or " + f" {ConditionMarker.__name__} subclasses." + ) cls.marker_class_to_tag[marker_class] = positive_tag @@ -132,7 +139,7 @@ class InvalidMarkerConfig(RasaException): @dataclass class EventMetaData: - """Describes meta data per event in some dialogue.""" + """Describes meta data per event in some session.""" idx: int preceding_user_turns: int @@ -161,8 +168,11 @@ def __init__(self, name: Optional[Text] = None, negated: bool = False) -> None: conversion of this marker negated: whether this marker should be negated (i.e. a negated marker applies if and only if the non-negated marker does not apply) + Raises: + `InvalidMarkerConfig` if the chosen *name* of the marker is the tag of + a predefined marker. """ - if name == Marker.ANY_MARKER: + if name == Marker.ANY_MARKER or name in MarkerRegistry.all_tags: raise InvalidMarkerConfig( f"You must not use the special marker name {Marker.ANY_MARKER}. " f"This is to avoid confusion when you generate a marker from a " @@ -172,24 +182,45 @@ def __init__(self, name: Optional[Text] = None, negated: bool = False) -> None: ) self.name = name self.history: List[bool] = [] + # Note: we allow negation here even though there might not be a negated tag + # for 2 reasons: testing and the fact that the `MarkerRegistry`+`from_config` + # won't allow to create a negated marker if there is not negated tag. self.negated: bool = negated def __str__(self) -> Text: return self.name or repr(self) def __repr__(self) -> Text: - tag = str(self.negated_tag()) if self.negated else self.tag() - return self._to_str_with(tag) + return self._to_str_with(self.get_tag()) + + def get_tag(self) -> Text: + """Returns the tag describing this marker.""" + if self.negated: + tag = self.negated_tag() + if tag is None: + # We allow the instantiation of a negated marker even if there + # is no negated tag (ie. direct creation of the negated marker from + # a config is not allowed). To be able to print a tag nonetheless: + tag = f"not({self.positive_tag()})" + return tag + else: + return self.positive_tag() @staticmethod @abstractmethod - def tag() -> Text: + def positive_tag() -> Text: """Returns the tag to be used in a config file.""" ... @staticmethod def negated_tag() -> Optional[Text]: - """Returns the tag to be used in a config file for the negated version.""" + """Returns the tag to be used in a config file for the negated version. + + If this maps to `None`, then this indicates that we do not allow a short-cut + for negating this marker. Hence, there is not a single tag to instantiate + a negated version of this marker. One must use a "not" in the configuration + file then. + """ return None @abstractmethod @@ -237,6 +268,15 @@ def __len__(self) -> int: """Returns the count of all markers that are part of this marker.""" ... + @abstractmethod + def validate_against_domain(self, domain: Domain) -> bool: + """Checks that this marker (and its children) refer to entries in the domain. + + Args: + domain: The domain to check against + """ + ... + def evaluate_events( self, events: List[Event], recursive: bool = False ) -> List[Dict[Text, List[EventMetaData]]]: @@ -256,30 +296,29 @@ def evaluate_events( recursive: set this to `True` to collect evaluations for all markers that this marker consists of Returns: - a list that contains, for each dialogue contained in the tracker, a + a list that contains, for each session contained in the tracker, a dictionary mapping that maps marker names to meta data of relevant events """ # determine which marker to extract results from - markers_to_be_evaluated: List[Marker] = [] if recursive: markers_to_be_evaluated = [marker for marker in self] - elif isinstance(self, CompoundMarker) and self.name == Marker.ANY_MARKER: + elif isinstance(self, OperatorMarker) and self.name == Marker.ANY_MARKER: markers_to_be_evaluated = self.sub_markers else: markers_to_be_evaluated = [self] - # split the events into dialogues and evaluate them separately - dialogues_and_start_indices = self._split_sessions(events=events) + # split the events into sessions and evaluate them separately + sessions_and_start_indices = self._split_sessions(events=events) extracted_markers: List[Dict[Text, List[EventMetaData]]] = [] - for dialogue, start_idx in dialogues_and_start_indices: - # track all events and collect meta data per timestep + for session, start_idx in sessions_and_start_indices: + # track all events and collect meta data per time step meta_data = self._track_all_and_collect_meta_data( - events=dialogue, event_idx_offset=start_idx + events=session, event_idx_offset=start_idx ) # for each marker, keep only certain meta data - extracted: Dict[Text, EventMetaData] = { + extracted: Dict[Text, List[EventMetaData]] = { str(marker): [meta_data[idx] for idx in marker.relevant_events()] for marker in markers_to_be_evaluated } @@ -288,7 +327,7 @@ def evaluate_events( @staticmethod def _split_sessions(events: List[Event]) -> List[Tuple[List[Event], int]]: - """Identifies single dialogues in a the given sequence of events. + """Identifies single sessions in a the given sequence of events. Args: events: a sequence of events, e.g. extracted from a tracker store @@ -306,19 +345,19 @@ def _split_sessions(events: List[Event]) -> List[Tuple[List[Event], int]]: ] if len(session_start_indices) == 0: return [(events, 0)] - dialogues_and_start_indices: List[Tuple[List[Event], int]] = [] - for dialogue_idx in range(len(session_start_indices)): + sessions_and_start_indices: List[Tuple[List[Event], int]] = [] + for session_idx in range(len(session_start_indices)): start_idx = ( - session_start_indices[dialogue_idx - 1] if (dialogue_idx > 0) else 0 + session_start_indices[session_idx - 1] if (session_idx > 0) else 0 ) - end_idx = session_start_indices[dialogue_idx] - dialogue = [events[idx] for idx in range(start_idx, end_idx)] - dialogues_and_start_indices.append((dialogue, start_idx)) - last_dialogue = [ + end_idx = session_start_indices[session_idx] + session = [events[idx] for idx in range(start_idx, end_idx)] + sessions_and_start_indices.append((session, start_idx)) + last_session = [ events[idx] for idx in range(session_start_indices[-1], len(events)) ] - dialogues_and_start_indices.append((last_dialogue, session_start_indices[-1])) - return dialogues_and_start_indices + sessions_and_start_indices.append((last_session, session_start_indices[-1])) + return sessions_and_start_indices def _track_all_and_collect_meta_data( self, events: List[Event], event_idx_offset: int = 0 @@ -326,7 +365,7 @@ def _track_all_and_collect_meta_data( """Resets the marker, tracks all events, and collects metadata. Args: - events: all events of a *single* dialogue that should be tracked and + events: all events of a *single* session that should be tracked and evaluated event_idx_offset: offset that will be used to modify the collected event meta data, i.e. all event indices will be shifted by this offset @@ -335,19 +374,19 @@ def _track_all_and_collect_meta_data( given `event_idx_offset` """ self.reset() - dialogue_meta_data: List[EventMetaData] = [] - num_preceeding_user_turns = 0 + session_meta_data: List[EventMetaData] = [] + num_preceding_user_turns = 0 for idx, event in enumerate(events): is_user_turn = isinstance(event, UserUttered) - dialogue_meta_data.append( + session_meta_data.append( EventMetaData( idx=idx + event_idx_offset, - preceding_user_turns=num_preceeding_user_turns, + preceding_user_turns=num_preceding_user_turns, ) ) self.track(event=event) - num_preceeding_user_turns += int(is_user_turn) - return dialogue_meta_data + num_preceding_user_turns += int(is_user_turn) + return session_meta_data def relevant_events(self) -> List[int]: """Returns the indices of those tracked events that are relevant for evaluation. @@ -361,11 +400,25 @@ def relevant_events(self) -> List[int]: """ return [idx for (idx, applies) in enumerate(self.history) if applies] - @staticmethod - def from_path(path: Union[Path, Text]) -> Marker: + @classmethod + def from_path(cls, path: Union[Path, Text]) -> Marker: """Loads markers from one config file or all config files in a directory tree. - For more details, see `from_config_dict`. + Each config file should contain a dictionary mapping marker names to the + respective marker configuration. + To avoid confusion, the marker names must not coincide with the tag of + any pre-defined markers. Moreover, marker names must be unique. This means, + if you you load the markers from multiple files, then you have to make sure + that the names of the markers defined in these files are unique across all + loaded files. + + Note that all loaded markers will be combined into one marker via one + artificial OR-operator. When evaluating the resulting marker, then the + artificial OR-operator will be ignored and results will be reported for + every sub-marker. + + For more details how a single marker configuration looks like, have a look + at `Marker.from_config`. Args: path: either the path to a single config file or the root of the directory @@ -373,75 +426,118 @@ def from_path(path: Union[Path, Text]) -> Marker: Returns: all configured markers, combined into one marker object """ + MarkerRegistry.register_builtin_markers() + from rasa.core.evaluation.marker import OrMarker + + # collect all the files from which we want to load configs (i.e. either just + # the given path if points to a yaml or all yaml-files in the directory tree) + yaml_files = cls._collect_yaml_files_from_path(path) + + # collect all the configs from those yaml files (assure it's dictionaries + # mapping marker names to something) -- keep track of which config came + # from which file to be able to raise meaningful errors later + loaded_configs: Dict[Text, Dict] = cls._collect_configs_from_yaml_files( + yaml_files + ) + + # create markers from all loaded configurations + loaded_markers: List[Marker] = [] + for yaml_file, config in loaded_configs.items(): + for marker_name, marker_config in config.items(): + try: + marker = Marker.from_config(marker_config, name=marker_name) + except InvalidMarkerConfig as e: + raise InvalidMarkerConfig( + f"Could not load marker {marker_name} from {yaml_file}" + ) from e + loaded_markers.append(marker) + + # combine the markers + if len(loaded_markers) > 1: + marker = OrMarker(markers=loaded_markers) + marker.name = Marker.ANY_MARKER # cannot be set via name parameter + else: + marker = loaded_markers[0] + + return marker + + @staticmethod + def _collect_yaml_files_from_path(path: Union[Text, Path]) -> List[Text]: path = os.path.abspath(path) if os.path.isfile(path): - config = rasa.shared.utils.io.read_yaml_file(path) + yaml_files = [ + yaml_file for yaml_file in [path] if is_likely_yaml_file(yaml_file) + ] + if not yaml_files: + raise InvalidMarkerConfig(f"Could not find a yaml file at '{path}'.") elif os.path.isdir(path): - config = Marker._load_and_combine_config_files_under(root_dir=path) + yaml_files = [ + os.path.join(root, file) + for root, _, files in os.walk(path, followlinks=True) + for file in files + if is_likely_yaml_file(file) + ] + if not yaml_files: + raise InvalidMarkerConfig( + f"Could not find any yaml in the directory tree rooted at '{path}'." + ) else: - config = {} - if not config: - raise InvalidMarkerConfig(f"Could not load any markers from '{path}'.") - return Marker.from_config_dict(config) + raise InvalidMarkerConfig( + f"The given path ({path}) is neither pointing to a directory " + f"nor a file. Please specify the location of a yaml file or a " + f"root directory (all yaml configs found in the directories " + f"under that root directory will be loaded." + ) # TODO: add link to docs + return yaml_files @staticmethod - def _load_and_combine_config_files_under(root_dir: Text) -> MarkerConfig: - combined_configs = {} - yaml_files = [ - os.path.join(root, file) - for root, _, files in os.walk(root_dir, followlinks=True) - for file in files - if is_likely_yaml_file(file) - ] + def _collect_configs_from_yaml_files(yaml_files: List[Text]) -> Dict[Text, Dict]: + marker_names = set() + loaded_configs: Dict[Text, Dict] = {} for yaml_file in yaml_files: - config = rasa.shared.utils.io.read_yaml_file(yaml_file) - # TODO: validation - if set(config.keys()).intersection(combined_configs.keys()): + loaded_config = rasa.shared.utils.io.read_yaml_file(yaml_file) + if not isinstance(loaded_config, dict): + raise InvalidMarkerConfig( + f"Expected the loaded configurations to be a dictionary " + f"of marker configurations but found a " + f"{type(loaded_config)} in {yaml_file}." + ) # TODO: add link to docs + if set(loaded_config.keys()).intersection(marker_names): raise InvalidMarkerConfig( f"The names of markers defined in {yaml_file} " - f"({sorted(config.keys())}) " + f"({sorted(loaded_config.keys())}) " f"overlap with the names of markers loaded so far " - f"({sorted(combined_configs.keys())})." - ) - combined_configs.extend(config) - logging.info(f"Added markers from {yaml_file}") - return combined_configs - - @staticmethod - def from_config_dict(config: Dict[Text, MarkerConfig]) -> Marker: - """Creates markers from a dictionary of marker configurations. - - If there is more than one custom marker defined in the given dictionary, - then the returned marker will be an `or` combination of all defined markers - named `ANY_MARKER`. - During evaluation, where we usually only return results for the top-level - marker, we identify this special marker by it's name and return evaluations - for all combined markers instead. - - Args: - config: mapping custom marker names to marker configurations - Returns: - all configured markers, combined into one marker - """ - from rasa.core.evaluation.marker import OrMarker - - markers = [ - Marker.from_config(marker_config, name=marker_name) - for marker_name, marker_config in config.items() - ] - if len(markers) > 1: - marker = OrMarker(markers=markers) - marker.name = Marker.ANY_MARKER # cannot be set via name parameter - else: - marker = markers[0] - return marker + f"({sorted(marker_names)}). " + f"Please adapt your configurations such that your custom " + f"marker names are unique." + ) # TODO: add link to docs + if set(loaded_config.keys()).intersection(MarkerRegistry.all_tags): + raise InvalidMarkerConfig( + f"The top level of your marker configuration should consist " + f"of names for your custom markers. " + f"If you use a condition or operator name at the top level, " + f"then that will not be recognised as an actual condition " + f"or operator and won't be evaluated against any sessions. " + f"Please remove any of the pre-defined marker tags " + f"(i.e. {MarkerRegistry.all_tags}) " + f"from {yaml_file}." + ) # TODO: add link to docs + marker_names.update(loaded_config.keys()) + loaded_configs[yaml_file] = loaded_config + return loaded_configs @staticmethod - def from_config(config: MarkerConfig, name: Optional[Text] = None) -> Marker: + def from_config(config: Any, name: Optional[Text] = None) -> Marker: """Creates a marker from the given config. + A marker configuration is a dictionary mapping a marker tag (either a + `positive_tag` or a `negated_tag`) to a sub-configuration. + How that sub-configuration looks like, depends on whether the tag describes + an operator (see `OperatorMarker.from_tag_and_sub_config`) or a + condition (see `ConditionMarker.from_tag_and_sub_config`). + Args: - config: the configuration of a single or multiple markers + config: a marker configuration name: a custom name that will be used for the top-level marker (if and only if there is only one top-level marker) @@ -451,42 +547,35 @@ def from_config(config: MarkerConfig, name: Optional[Text] = None) -> Marker: # Triggers the import of all modules containing marker classes in order to # register all configurable markers. MarkerRegistry.register_builtin_markers() - from rasa.core.evaluation.marker import AndMarker - - # A marker config can be either an atomic marker config list or a - # compound marker configuration - if it is not a list, we - marker_configs = config if isinstance(config, list) else [config] - - collected_sub_markers = [] - for marker_config in marker_configs: - - # FIXME: check that this is basically a tuple - marker_name = next(iter(marker_config)) - sub_marker_config = marker_config[marker_name] - - tag, _ = MarkerRegistry.get_non_negated_tag(tag_or_negated_tag=marker_name) - if tag in MarkerRegistry.operator_tag_to_marker_class: - sub_markers = [ - CompoundMarker.from_config( - operator=marker_name, - sub_marker_configs=sub_marker_config, - name=name, - ) - ] - else: - sub_markers = AtomicMarker.from_config( - marker_name=marker_name, - sub_marker_config=sub_marker_config, - name=name, - ) - collected_sub_markers.extend(sub_markers) - # Build the final marker - if len(collected_sub_markers) > 1: - marker = AndMarker(collected_sub_markers, name=name) + if not isinstance(config, dict) or len(config) != 1: + raise InvalidMarkerConfig( + "To configure a marker, please define a dictionary that maps a " + "single operator tag or a single condition tag to the " + "corresponding parameter configuration or a list of marker " + "configurations, respectively." + ) # TODO: add link to docs + + tag = next(iter(config)) + sub_marker_config = config[tag] + + tag, _ = MarkerRegistry.get_non_negated_tag(tag_or_negated_tag=tag) + if tag in MarkerRegistry.operator_tag_to_marker_class: + marker = OperatorMarker.from_tag_and_sub_config( + tag=tag, sub_config=sub_marker_config, name=name, + ) + elif tag in MarkerRegistry.condition_tag_to_marker_class: + marker = ConditionMarker.from_tag_and_sub_config( + tag=tag, sub_config=sub_marker_config, name=name, + ) else: - marker = collected_sub_markers[0] - marker.name = name + raise InvalidMarkerConfig( + f"Expected a marker configuration with a key that specifies" + f" an operator or a condition but found {tag}. " + f"Available conditions and operators are: " + f"{sorted(MarkerRegistry.all_tags)}. " + ) # TODO: add link to docs + return marker def export_markers( @@ -571,7 +660,7 @@ def _compute_stats( pass -class CompoundMarker(Marker, ABC): +class OperatorMarker(Marker, ABC): """Combines several markers into one.""" def __init__( @@ -585,9 +674,31 @@ def __init__( applies if and only if the non-negated marker does not apply) name: a custom name that can be used to replace the default string conversion of this marker + Raises: + `InvalidMarkerConfig` if the given number of sub-markers does not match + the expected number of sub-markers """ super().__init__(name=name, negated=negated) self.sub_markers: List[Marker] = markers + expected_num = self.expected_number_of_sub_markers() + if expected_num is not None and len(markers) != expected_num: + raise InvalidMarkerConfig( + f"Expected {expected_num} sub-marker(s) to be specified for marker " + f"'{self}' ({self.get_tag()}) but found {len(markers)}. " + f"Please adapt your configuration so that there are exactly " + f"{expected_num} sub-markers. " + ) + elif len(markers) == 0: + raise InvalidMarkerConfig( + f"Expected some sub-markers to be specified for {self} but " + f"found none. Please adapt your configuration so that there is " + f"at least one sub-marker. " + ) + + @staticmethod + def expected_number_of_sub_markers() -> Optional[int]: + """Returns the expected number of sub-markers (if there is any).""" + return None def _to_str_with(self, tag: Text) -> Text: marker_str = ", ".join(str(marker) for marker in self.sub_markers) @@ -621,68 +732,70 @@ def __len__(self) -> int: return len(self.sub_markers) + 1 def reset(self) -> None: - """Evaluate this marker given the next event. - - Args: - event: the next event of the conversation - """ + """Resets the history of this marker and all its sub-markers.""" for marker in self.sub_markers: marker.reset() super().reset() + def validate_against_domain(self, domain: Domain) -> bool: + """Checks that this marker (and its children) refer to entries in the domain. + + Args: + domain: The domain to check against + """ + return all( + marker.validate_against_domain(domain) for marker in self.sub_markers + ) + @staticmethod - def from_config( - operator: Text, - sub_marker_configs: List[Union[ConditionConfigList, OperatorConfig]], - name: Optional[Text] = None, - ) -> Marker: - """Creates a compound marker from the given config. + def from_tag_and_sub_config( + tag: Text, sub_config: Any, name: Optional[Text] = None, + ) -> OperatorMarker: + """Creates an operator marker from the given config. + + The configuration must consist of a list of marker configurations. + See `Marker.from_config` for more details. Args: - operator: a text identifying a compound marker type - sub_marker_configs: a list of configs defining sub-markers + tag: the tag identifying an operator + sub_config: a list of marker configs name: an optional custom name to be attached to the resulting marker Returns: - a compound marker if there are markers to combine - and just an event - marker if there is only a single marker + the configured operator marker + Raises: + `InvalidMarkerConfig` if the given config or the tag are not well-defined """ - tag, is_negation = MarkerRegistry.get_non_negated_tag(operator) - operator_class = MarkerRegistry.operator_tag_to_marker_class.get(tag) + positive_tag, is_negation = MarkerRegistry.get_non_negated_tag(tag) + operator_class = MarkerRegistry.operator_tag_to_marker_class.get(positive_tag) if operator_class is None: - raise InvalidConfigException(f"Unknown operator '{operator}'.") - - collected_sub_markers: List[Marker] = [] - for sub_marker_name_and_config in sub_marker_configs: - sub_marker_name = next(iter(sub_marker_name_and_config)) - sub_marker_config = sub_marker_name_and_config[sub_marker_name] + raise InvalidConfigException(f"Unknown operator '{tag}'.") - next_sub_markers: List[Marker] = [] - sub_tag, _ = MarkerRegistry.get_non_negated_tag( - tag_or_negated_tag=sub_marker_name + if not isinstance(sub_config, list): + raise InvalidMarkerConfig( + f"Expected a list of sub-marker configurations under {tag}." ) - if sub_tag in MarkerRegistry.condition_tag_to_marker_class: - # We allow several AtomicMarkers to be collected under the same tag. - # If this is done at the top-level (see `Marker.from_config`), then - # we would combine them under a new `AndMarker`. - # Here, we do *not* create a new `AndMarker` but add the single - # atomic markers to this compound marker. - next_sub_markers = AtomicMarker.from_config( - marker_name=sub_marker_name, sub_marker_config=sub_marker_config - ) - else: - next_sub_markers = [ - CompoundMarker.from_config( - operator=sub_marker_name, sub_marker_configs=sub_marker_config - ) - ] - collected_sub_markers.extend(next_sub_markers) - - marker = operator_class(markers=collected_sub_markers, negated=is_negation) + collected_sub_markers: List[Marker] = [] + for sub_marker_config in sub_config: + try: + sub_marker = Marker.from_config(sub_marker_config) + except InvalidMarkerConfig as e: + raise InvalidMarkerConfig( + f"Could not create sub-marker for operator '{tag}' from " + f"{sub_marker_config}" + ) from e + collected_sub_markers.append(sub_marker) + try: + marker = operator_class(markers=collected_sub_markers, negated=is_negation) + except InvalidMarkerConfig as e: + raise InvalidMarkerConfig( + f"Could not create operator '{tag}' with sub-markers " + f"{collected_sub_markers}" + ) from e marker.name = name return marker -class AtomicMarker(Marker, ABC): +class ConditionMarker(Marker, ABC): """A marker that does not contain any sub-markers.""" def __init__( @@ -703,7 +816,7 @@ def __init__( def _to_str_with(self, tag: Text) -> Text: return f"({tag}: {self.text})" - def __iter__(self) -> Iterator[AtomicMarker]: + def __iter__(self) -> Iterator[ConditionMarker]: """Returns an iterator that just returns this `AtomicMarker`. Returns: @@ -716,34 +829,31 @@ def __len__(self) -> int: return 1 @staticmethod - def from_config( - marker_name: Text, - sub_marker_config: Union[Text, List[Text]], - name: Optional[Text] = None, - ) -> List[AtomicMarker]: + def from_tag_and_sub_config( + tag: Text, sub_config: Any, name: Optional[Text] = None + ) -> ConditionMarker: """Creates an atomic marker from the given config. Args: - marker_name: string identifying an atomic marker type - sub_marker_config: a list of texts or just one text which should be - used to instantiate the condition marker(s) + tag: the tag identifying a condition + sub_config: a single text parameter expected by all condition markers; + e.g. if the tag is for an `intent_detected` marker then the `config` + should contain an intent name name: a custom name for this marker Returns: - the configured `AtomicMarker`s + the configured `ConditionMarker` + Raises: + `InvalidMarkerConfig` if the given config or the tag are not well-defined """ - tag, is_negation = MarkerRegistry.get_non_negated_tag(marker_name) - marker_class = MarkerRegistry.condition_tag_to_marker_class.get(tag) + positive_tag, is_negation = MarkerRegistry.get_non_negated_tag(tag) + marker_class = MarkerRegistry.condition_tag_to_marker_class.get(positive_tag) if marker_class is None: - raise InvalidConfigException(f"Unknown condition '{marker_name}'.") + raise InvalidConfigException(f"Unknown condition '{tag}'.") - if isinstance(sub_marker_config, Text): - sub_marker_config_list = [sub_marker_config] - else: - sub_marker_config_list = sub_marker_config - - markers = [] - for marker_text in sub_marker_config_list: - marker = marker_class(marker_text, negated=is_negation) - marker.name = name - markers.append(marker) - return markers + if not isinstance(sub_config, str): + raise InvalidMarkerConfig( + f"Expected a text parameter to be specified for marker '{tag}'." + ) + marker = marker_class(sub_config, negated=is_negation) + marker.name = name + return marker diff --git a/tests/cli/test_rasa_evaluate_markers.py b/tests/cli/test_rasa_evaluate_markers.py index d33b68b3c7bd..ff23cb52890d 100644 --- a/tests/cli/test_rasa_evaluate_markers.py +++ b/tests/cli/test_rasa_evaluate_markers.py @@ -119,7 +119,14 @@ def test_markers_cli_results_save_correctly( results_path = tmp_path / "results.csv" rasa.cli.evaluate._run_markers( - None, 10, endpoints_path, "first_n", markers_path, results_path, None + seed=None, + count=10, + endpoint_config=endpoints_path, + strategy="first_n", + domain_path=None, + config=markers_path, + output_filename=results_path, + stats_file=None, ) with open(results_path, "r") as results: diff --git a/tests/conftest.py b/tests/conftest.py index 465746acd0fa..8de3d38243cd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -168,36 +168,6 @@ def endpoints_path() -> Text: return "data/test_endpoints/example_endpoints.yml" -@pytest.fixture(scope="session") -def simple_markers_config() -> Text: - return "data/test_markers/config_simple.yml" - - -@pytest.fixture(scope="session") -def markers_config_folder() -> Text: - return "data/test_markers/config_dir" - - -@pytest.fixture(scope="session") -def invalid_markers_config() -> Text: - return "data/test_markers/config_invalid.yml" - - -@pytest.fixture(scope="session") -def markers_config_operators() -> Text: - return "data/test_markers/config_operators.yml" - - -@pytest.fixture(scope="session") -def extracted_markers_json() -> Text: - return "data/test_markers/extracted_markers.json" - - -@pytest.fixture(scope="session") -def marker_stats_output_json() -> Text: - return "stats_output.json" - - # https://github.com/pytest-dev/pytest-asyncio/issues/68 # this event_loop is used by pytest-asyncio, and redefining it # is currently the only way of changing the scope of this fixture diff --git a/tests/core/evaluation/test_marker.py b/tests/core/evaluation/test_marker.py index 24382d7621b0..b2c24acdb038 100644 --- a/tests/core/evaluation/test_marker.py +++ b/tests/core/evaluation/test_marker.py @@ -1,114 +1,79 @@ import csv -from rasa.core.evaluation.marker_tracker_loader import MarkerTrackerLoader -from rasa.shared.core.trackers import DialogueStateTracker -from rasa.core.tracker_store import InMemoryTrackerStore -from rasa.shared.core.domain import Domain -from typing import List, Optional, Text, Tuple, Type +from typing import List, Optional, Set, Text, Tuple, Type, Any import itertools +from pathlib import Path import pytest import numpy as np -import rasa.shared.utils.schemas.markers +import rasa.shared.utils.io from rasa.core.evaluation.marker import ( + IntentDetectedMarker, + SlotSetMarker, + OccurrenceMarker, ActionExecutedMarker, AndMarker, - IntentDetectedMarker, OrMarker, - SlotSetMarker, + NotMarker, SequenceMarker, - OccurrenceMarker, ) from rasa.core.evaluation.marker_base import ( - CompoundMarker, + OperatorMarker, Marker, - AtomicMarker, + ConditionMarker, InvalidMarkerConfig, ) from rasa.shared.core.constants import ACTION_SESSION_START_NAME from rasa.shared.core.events import SlotSet, ActionExecuted, UserUttered from rasa.shared.nlu.constants import INTENT_NAME_KEY - +from rasa.shared.core.slots import Slot +from rasa.core.evaluation.marker_tracker_loader import MarkerTrackerLoader +from rasa.shared.core.trackers import DialogueStateTracker +from rasa.core.tracker_store import InMemoryTrackerStore +from rasa.shared.core.domain import Domain CONDITION_MARKERS = [ActionExecutedMarker, SlotSetMarker, IntentDetectedMarker] OPERATOR_MARKERS = [AndMarker, OrMarker, SequenceMarker, OccurrenceMarker] -def test_marker_from_config_dict_single_and(): +def test_marker_from_config(): config = { - "marker_1": { - AndMarker.tag(): [ - {SlotSetMarker.tag(): ["s1"]}, - { - OrMarker.tag(): [ - {IntentDetectedMarker.tag(): ["4"]}, - {IntentDetectedMarker.negated_tag(): ["6"]}, - ] - }, - ] - } + AndMarker.positive_tag(): [ + {SlotSetMarker.positive_tag(): "s1"}, + { + OrMarker.positive_tag(): [ + {IntentDetectedMarker.positive_tag(): "4"}, + {IntentDetectedMarker.negated_tag(): "6"}, + ] + }, + ] } - marker = Marker.from_config_dict(config) - assert marker.name == "marker_1" - assert isinstance(marker, AndMarker) - assert isinstance(marker.sub_markers[0], SlotSetMarker) - assert isinstance(marker.sub_markers[1], OrMarker) - for sub_marker in marker.sub_markers[1].sub_markers: - assert isinstance(sub_marker, AtomicMarker) - - -def test_marker_from_config_list_inserts_and_marker(): - config = [ - {SlotSetMarker.tag(): ["s1"]}, - { - OrMarker.tag(): [ - {IntentDetectedMarker.tag(): ["4"]}, - {IntentDetectedMarker.negated_tag(): ["6"]}, - ] - }, - ] + marker = Marker.from_config(config) - assert isinstance(marker, AndMarker) # i.e. the default marker inserted + assert isinstance(marker, AndMarker) assert isinstance(marker.sub_markers[0], SlotSetMarker) - assert isinstance(marker.sub_markers[1], OrMarker) - for sub_marker in marker.sub_markers[1].sub_markers: - assert isinstance(sub_marker, AtomicMarker) - - -def test_marker_from_config_unwraps_grouped_conditions_under_compound(): - config = [ - { - OrMarker.tag(): [ - {IntentDetectedMarker.tag(): ["1", "2"]}, - {IntentDetectedMarker.negated_tag(): ["3", "4", "5"]}, - ] - }, - ] - marker = Marker.from_config(config) - assert isinstance(marker, OrMarker) - assert len(marker.sub_markers) == 5 - assert all( - isinstance(sub_marker, IntentDetectedMarker) - for sub_marker in marker.sub_markers - ) - assert set(sub_marker.text for sub_marker in marker.sub_markers) == { - str(i + 1) for i in range(5) - } + or_marker = marker.sub_markers[1] + assert isinstance(or_marker, OrMarker) + for sub_marker in or_marker.sub_markers: + assert isinstance(sub_marker, ConditionMarker) @pytest.mark.parametrize("marker_class", CONDITION_MARKERS) -def test_atomic_markers_negated_to_str(marker_class: Type[AtomicMarker]): +def test_condition_negated_to_str(marker_class: Type[ConditionMarker]): marker = marker_class("intent1", negated=True) if marker.negated_tag() is not None: assert marker.negated_tag() in str(marker) @pytest.mark.parametrize( - "atomic_marker_type, negated", itertools.product(CONDITION_MARKERS, [False, True]) + "condition_marker_type, negated", + itertools.product(CONDITION_MARKERS, [False, True]), ) -def test_atomic_marker_track(atomic_marker_type: Type[AtomicMarker], negated: bool): +def test_condition(condition_marker_type: Type[ConditionMarker], negated: bool): """Each marker applies an exact number of times (slots are immediately un-set).""" - marker = atomic_marker_type(text="same-text", name="marker_name", negated=negated) + marker = condition_marker_type( + text="same-text", name="marker_name", negated=negated + ) events = [ UserUttered(intent={"name": "1"}), UserUttered(intent={"name": "same-text"}), @@ -129,8 +94,8 @@ def test_atomic_marker_track(atomic_marker_type: Type[AtomicMarker], negated: bo assert sum(marker.history) == expected -@pytest.mark.parametrize("atomic_marker_type", CONDITION_MARKERS) -def test_atomic_marker_evaluate_events(atomic_marker_type: Type[AtomicMarker]): +@pytest.mark.parametrize("condition_marker_type", CONDITION_MARKERS) +def test_condition_evaluate_events(condition_marker_type: Type[ConditionMarker]): """Each marker applies an exact number of times (slots are immediately un-set).""" events = [ UserUttered(intent={INTENT_NAME_KEY: "1"}), @@ -141,30 +106,51 @@ def test_atomic_marker_evaluate_events(atomic_marker_type: Type[AtomicMarker]): ] num_applies = 3 events = events * num_applies - marker = atomic_marker_type(text="same-text", name="marker_name") + marker = condition_marker_type(text="same-text", name="marker_name") evaluation = marker.evaluate_events(events) assert len(evaluation) == 1 assert "marker_name" in evaluation[0] - if atomic_marker_type == IntentDetectedMarker: + if condition_marker_type == IntentDetectedMarker: expected = [1, 3, 5] else: expected = [2, 4, 6] - actual_preceeding_user_turns = [ + actual_preceding_user_turns = [ meta_data.preceding_user_turns for meta_data in evaluation[0]["marker_name"] ] - assert actual_preceeding_user_turns == expected + assert actual_preceding_user_turns == expected @pytest.mark.parametrize("marker_class", OPERATOR_MARKERS) -def test_compound_markers_negated_to_str(marker_class: Type[CompoundMarker]): +def test_operator_negated_to_str(marker_class: Type[OperatorMarker]): marker = marker_class([IntentDetectedMarker("bla")], negated=True) if marker.negated_tag() is not None: assert marker.negated_tag() in str(marker) +@pytest.mark.parametrize( + "operator_class, negated", + [ + (operator_class, negated) + for operator_class, negated in itertools.product( + OPERATOR_MARKERS, [True, False] + ) + if operator_class.expected_number_of_sub_markers() is not None + ], +) +def test_operator_raises_wrong_amount_sub_markers( + operator_class: Type[OperatorMarker], negated: bool +): + expected_number = operator_class.expected_number_of_sub_markers() + one_more_than_expected = [ + IntentDetectedMarker("bla") for _ in range(expected_number + 1) + ] + with pytest.raises(InvalidMarkerConfig): + operator_class(one_more_than_expected, name="marker_name", negated=negated) + + @pytest.mark.parametrize("negated", [True, False]) -def test_compound_marker_or_track(negated: bool): +def test_operator_or(negated: bool): events = [ UserUttered(intent={INTENT_NAME_KEY: "1"}), UserUttered(intent={INTENT_NAME_KEY: "unknown"}), @@ -182,7 +168,23 @@ def test_compound_marker_or_track(negated: bool): @pytest.mark.parametrize("negated", [True, False]) -def test_compound_marker_and_track(negated: bool): +def test_operator_not(negated: bool): + events = [ + UserUttered(intent={INTENT_NAME_KEY: "1"}), + UserUttered(intent={INTENT_NAME_KEY: "unknown"}), + ] + sub_markers = [IntentDetectedMarker("1")] + marker = NotMarker(sub_markers, name="marker_name", negated=negated) + for event in events: + marker.track(event) + expected = [False, True] + if negated: + expected = [not applies for applies in expected] + assert marker.history == expected + + +@pytest.mark.parametrize("negated", [True, False]) +def test_operator_and(negated: bool): events_expected = [ (UserUttered(intent={INTENT_NAME_KEY: "1"}), False), (SlotSet("2", value="bla"), False), @@ -204,7 +206,7 @@ def test_compound_marker_and_track(negated: bool): @pytest.mark.parametrize("negated", [True, False]) -def test_compound_marker_seq_track(negated: bool): +def test_operator_seq_track(negated: bool): events_expected = [ (UserUttered(intent={INTENT_NAME_KEY: "1"}), False), (UserUttered(intent={INTENT_NAME_KEY: "2"}), True), @@ -224,7 +226,7 @@ def test_compound_marker_seq_track(negated: bool): @pytest.mark.parametrize("negated", [True, False]) -def test_compound_marker_occur_track(negated: bool): +def test_operator_occur(negated: bool): events_expected = [ (UserUttered(intent={INTENT_NAME_KEY: "0"}), False), (SlotSet("2", value=None), False), @@ -251,20 +253,7 @@ def test_compound_marker_occur_track(negated: bool): assert marker.relevant_events() == [expected.index(True)] -@pytest.mark.parametrize( - "sub_markers, negated", - itertools.product( - ([], [IntentDetectedMarker("1"), IntentDetectedMarker("2")]), [False, True] - ), -) -def test_compound_marker_occur_raises_wrong_amount_submarkers( - sub_markers: List[Marker], negated: bool -): - with pytest.raises(InvalidMarkerConfig): - OccurrenceMarker(sub_markers, name="marker_name", negated=negated) - - -def test_compound_marker_occur_never_applied(): +def test_operator_occur_never_applied(): events_expected = [ (UserUttered(intent={INTENT_NAME_KEY: "2"}), False), (SlotSet("2", value=None), False), @@ -277,14 +266,14 @@ def test_compound_marker_occur_never_applied(): name="and marker", negated=False, ) - marker = OccurrenceMarker([sub_marker], name="or", negated=False) + marker = OccurrenceMarker([sub_marker], name="or_at_some_point", negated=False) for event in events: marker.track(event) assert marker.relevant_events() == [] -def test_compound_marker_occur_never_applied_negated(): +def test_operator_occur_never_applied_negated(): events_expected = [ (UserUttered(intent={INTENT_NAME_KEY: "1"}), False), (SlotSet("2", value=None), False), @@ -294,17 +283,17 @@ def test_compound_marker_occur_never_applied_negated(): events, expected = zip(*events_expected) sub_marker = OrMarker( [IntentDetectedMarker("1"), SlotSetMarker("2")], - name="and marker", + name="or marker", negated=False, ) - marker = OccurrenceMarker([sub_marker], name="or", negated=True) + marker = OccurrenceMarker([sub_marker], name="or never occurred", negated=True) for event in events: marker.track(event) assert marker.relevant_events() == [] -def test_compound_marker_nested_simple_track(): +def test_operators_nested_simple(): events = [ UserUttered(intent={"name": "1"}), UserUttered(intent={"name": "2"}), @@ -333,9 +322,9 @@ def generate_random_marker( max_branches: int, rng: np.random.Generator, constant_condition_text: Optional[Text], - possible_conditions: List[Type[AtomicMarker]], + possible_conditions: List[Type[ConditionMarker]], constant_negated: Optional[bool], - possible_operators: List[Type[CompoundMarker]], + possible_operators: List[Type[OperatorMarker]], ) -> Tuple[Marker, int]: """Generates an (max_branches)-ary tree with the specified depth.""" if depth == 0: @@ -344,14 +333,11 @@ def generate_random_marker( condition_text = constant_condition_text or f"{rng.choice(1000)}" return condition_class(text=condition_text, negated=negated), 1 else: + negated = bool(rng.choice(2)) if constant_negated is None else constant_negated operator_class = possible_operators[rng.choice(len(possible_operators))] - # TODO: Causes error with the `Occurrence` marker as this one doesn't allow - # more than one sub markers. Revisit this when handling the config validation. - num_branches = ( - 1 - if operator_class == OccurrenceMarker - else rng.choice(max_branches - 1) + 1 - ) + num_branches = operator_class.expected_number_of_sub_markers() + if num_branches is None: + num_branches = rng.choice(max_branches - 1) + 1 marker_size = 0 sub_markers = [] for _ in range(num_branches): @@ -366,7 +352,7 @@ def generate_random_marker( ) marker_size += sub_marker_size sub_markers.append(sub_marker) - negated = bool(rng.choice(2)) if constant_negated is None else constant_negated + marker = operator_class(markers=sub_markers, negated=negated) marker_size += 1 return marker, marker_size @@ -375,7 +361,7 @@ def generate_random_marker( @pytest.mark.parametrize( "depth, max_branches, seed", [(1, 3, 3456), (4, 3, 345), (4, 5, 2345)] ) -def test_compound_marker_nested_randomly_track( +def test_operator_nested_randomly_all_sub_markers_track_events( depth: int, max_branches: int, seed: int ): rng = np.random.default_rng(seed=seed) @@ -403,18 +389,18 @@ def test_compound_marker_nested_randomly_track( @pytest.mark.parametrize( - "depth, seed, matches", + "depth, seed, applies_at_some_point", [ - (depth, seed, matches) + (depth, seed, applies_at_some_point) for depth, seed in [(1, 3456), (4, 345)] - for matches in [True, False] + for applies_at_some_point in [True, False] ], ) -def test_compound_marker_nested_randomly_ors_track( - depth: int, seed: int, matches: bool +def test_operator_nested_randomly_all_sub_markers_track_events_and_apply_at_some_point( + depth: int, seed: int, applies_at_some_point: bool ): rng = np.random.default_rng(seed=seed) - constant_condition_text = "1" if matches else "2" + constant_condition_text = "1" if applies_at_some_point else "2" marker, _ = generate_random_marker( depth=depth, max_branches=3, @@ -436,7 +422,7 @@ def test_compound_marker_nested_randomly_ors_track( marker.track(event) # by design, every marker applies at some point / never - if matches: + if applies_at_some_point: assert all([any(sub_marker.history) for sub_marker in marker]) else: assert all([not any(sub_marker.history) for sub_marker in marker]) @@ -486,12 +472,7 @@ def test_sessions_evaluated_returns_event_indices_wrt_tracker_not_dialogue(): assert evaluation[1]["my-marker"][0].idx == 7 # i.e. NOT the index in the dialogue -def test_atomic_markers_repr_not(): - marker = IntentDetectedMarker("intent1", negated=True) - assert str(marker) == "(intent_not_detected: intent1)" - - -def test_markers_cli_results_save_correctly(tmp_path: Text): +def test_markers_cli_results_save_correctly(tmp_path: Path): domain = Domain.empty() store = InMemoryTrackerStore(domain) @@ -509,8 +490,8 @@ def test_markers_cli_results_save_correctly(tmp_path: Text): results_path = tmp_path / "results.csv" - markers = Marker.from_config_dict( - {"marker1": {"slot_is_set": "2"}, "marker2": {"slot_is_set": "7"}} + markers = OrMarker( + markers=[SlotSetMarker("2", name="marker1"), SlotSetMarker("7", name="marker2")] ) markers.export_markers(tracker_loader.load(), results_path, stats_file=None) @@ -533,14 +514,145 @@ def test_markers_cli_results_save_correctly(tmp_path: Text): assert len(senders) == 5 -def test_all_operators_in_schema(): - operators_in_schema = rasa.shared.utils.schemas.markers.OPERATOR_SCHEMA["enum"] - operators_in_schema = {tag.lower() for tag in operators_in_schema} +def _collect_parameters( + marker: Marker, condition_type: Type[ConditionMarker] +) -> Set[Text]: + return set( + sub_marker.text + for sub_marker in marker + if isinstance(sub_marker, condition_type) + ) + + +@pytest.mark.parametrize( + "depth, max_branches, seed", [(1, 3, 3456), (4, 3, 345), (4, 5, 2345)] +) +def test_domain_validation_with_valid_marker(depth: int, max_branches: int, seed: int): + # We do this a bit backwards, we construct the domain from the marker + # and assert they must match + rng = np.random.default_rng(seed=seed) + marker, expected_size = generate_random_marker( + depth=depth, + max_branches=max_branches, + rng=rng, + possible_conditions=CONDITION_MARKERS, + possible_operators=OPERATOR_MARKERS, + constant_condition_text=None, + constant_negated=None, + ) + + slots = [Slot(name, []) for name in _collect_parameters(marker, SlotSetMarker)] + actions = list(_collect_parameters(marker, ActionExecutedMarker)) + intents = _collect_parameters(marker, IntentDetectedMarker) + domain = Domain(intents, [], slots, {}, actions, {}) + + assert marker.validate_against_domain(domain) + + +@pytest.mark.parametrize( + "depth, max_branches, seed", [(1, 3, 3456), (4, 3, 345), (4, 5, 2345)] +) +def test_domain_validation_with_invalid_marker( + depth: int, max_branches: int, seed: int +): + rng = np.random.default_rng(seed=seed) + marker, expected_size = generate_random_marker( + depth=depth, + max_branches=max_branches, + rng=rng, + possible_conditions=CONDITION_MARKERS, + possible_operators=OPERATOR_MARKERS, + constant_condition_text=None, + constant_negated=None, + ) + + domain = Domain.empty() + with pytest.warns(None): + is_valid = marker.validate_against_domain(domain) + assert not is_valid + + +@pytest.mark.parametrize( + "config", + [ + # (1) top level configuration + # - config is list + [SlotSetMarker.positive_tag()], + # - config is list with nested dict + [{SlotSetMarker.positive_tag(): "s1"}], + # - config is just a str + # (2) sub-markers + SlotSetMarker.positive_tag(), + # - sub-marker config is dict not list + {AndMarker.positive_tag(): {SlotSetMarker.positive_tag(): "s1"}}, + # - sub-marker under condition + {SlotSetMarker.positive_tag(): {IntentDetectedMarker.positive_tag(): "blah"}}, + # - no sub-config for operator (see also: "amount_sub_markers" tests) + {AndMarker.positive_tag(): "blah"}, + # (3) tags + # - unknown operator + {"Ard": {IntentDetectedMarker.positive_tag(): "intent1"}}, + # - unknown condition + {AndMarker.positive_tag(): {"intent": "intent1"}}, + # - special tag used by user + {Marker.ANY_MARKER: "blah"}, + ], +) +def test_marker_validation_raises(config: Any): + with pytest.raises(InvalidMarkerConfig): + Marker.from_config(config) + + +def test_marker_from_path_only_reads_yamls(tmp_path: Path,): + suffixes = [("yaml", True), ("yml", True), ("yaeml", False), ("config", False)] + for idx, (suffix, allowed) in enumerate(suffixes): + config = {f"marker-{idx}": {IntentDetectedMarker.positive_tag(): "intent"}} + config_file = tmp_path / f"config-{idx}.{suffix}" + rasa.shared.utils.io.write_yaml( + data=config, target=config_file, + ) + loaded = Marker.from_path(tmp_path) + assert len(loaded.sub_markers) == sum(allowed for _, allowed in suffixes) + assert set(sub_marker.name for sub_marker in loaded.sub_markers) == set( + f"marker-{idx}" for idx, (_, allowed) in enumerate(suffixes) if allowed + ) - actual_operators = set() - for operator in OPERATOR_MARKERS: - actual_operators.add(operator.tag()) - if operator.negated_tag(): - actual_operators.add(operator.negated_tag()) - assert actual_operators == operators_in_schema +@pytest.mark.parametrize( + "path_config_tuples", + [ + [ # two configs defining the same marker + ( + "folder1/config1.yaml", + {"my_marker1": {IntentDetectedMarker.positive_tag(): "this-intent"}}, + ), + ( + "folder2/config2.yaml", + { + "my_marker1": { + IntentDetectedMarker.positive_tag(): "different-intent" + }, + "my_marker2": {ActionExecutedMarker.positive_tag(): "action"}, + }, + ), + ], + [ # one buggy config + ( + "folder1/config2.yaml", + {"my_marker1": {IntentDetectedMarker.positive_tag(): "intent1"}}, + ), + ("folder2/config2.yaml", {"my_marker1": {"unknown-tag": "intent2"}}), + ], + ], +) +def test_marker_from_path_raises( + tmp_path: Path, path_config_tuples: List[Tuple[Text, Any]] +): + for path_to_yaml, config in path_config_tuples: + full_path = tmp_path / path_to_yaml + folder = full_path.parents[0] + if folder != tmp_path: + Path.mkdir(folder, exist_ok=False) + rasa.shared.utils.io.write_yaml(data=config, target=full_path) + with pytest.raises(InvalidMarkerConfig): + Marker.from_path(tmp_path)