From 008a6e9bd311a12b136bd55d6b17dc83fdb6debb Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Mon, 25 Sep 2023 16:50:13 +0100 Subject: [PATCH 01/10] update arg parser, implement validation checks, add tests --- rasa/cli/data.py | 16 ++ rasa/cli/utils.py | 11 +- rasa/shared/core/flows/flow.py | 2 +- rasa/validator.py | 136 ++++++++++++ tests/test_validator.py | 395 +++++++++++++++++++++++++++++++++ 5 files changed, 558 insertions(+), 2 deletions(-) diff --git a/rasa/cli/data.py b/rasa/cli/data.py index 06f912168c1d..4673a28e3783 100644 --- a/rasa/cli/data.py +++ b/rasa/cli/data.py @@ -144,6 +144,22 @@ def _add_data_validate_parsers( ) arguments.set_validator_arguments(story_structure_parser) + flows_structure_parser = validate_subparsers.add_parser( + "flows", + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + parents=parents, + help="Checks for inconsistencies in the flows files.", + ) + flows_structure_parser.set_defaults( + func=lambda args: rasa.cli.utils.validate_files( + args.fail_on_warnings, + args.max_history, + _build_training_data_importer(args), + flows_only=True, + ) + ) + arguments.set_validator_arguments(flows_structure_parser) + def _build_training_data_importer(args: argparse.Namespace) -> "TrainingDataImporter": config = rasa.cli.utils.get_validated_path( diff --git a/rasa/cli/utils.py b/rasa/cli/utils.py index b205c2072d17..c92f38cfe9e5 100644 --- a/rasa/cli/utils.py +++ b/rasa/cli/utils.py @@ -217,6 +217,7 @@ def validate_files( max_history: Optional[int], importer: TrainingDataImporter, stories_only: bool = False, + flows_only: bool = False, ) -> None: """Validates either the story structure or the entire project. @@ -225,6 +226,7 @@ def validate_files( max_history: The max history to use when validating the story structure. importer: The `TrainingDataImporter` to use to load the training data. stories_only: If `True`, only the story structure is validated. + flows_only: If `True`, only the flows are validated. """ from rasa.validator import Validator @@ -232,6 +234,8 @@ def validate_files( if stories_only: all_good = _validate_story_structure(validator, max_history, fail_on_warnings) + elif flows_only: + all_good = _validate_flows_structure(validator) else: if importer.get_domain().is_empty(): rasa.shared.utils.cli.print_error_and_exit( @@ -243,8 +247,9 @@ def validate_files( valid_stories = _validate_story_structure( validator, max_history, fail_on_warnings ) + valid_flows = _validate_flows_structure(validator) - all_good = valid_domain and valid_nlu and valid_stories + all_good = valid_domain and valid_nlu and valid_stories and valid_flows validator.warn_if_config_mandatory_keys_are_not_set() @@ -288,6 +293,10 @@ def _validate_story_structure( ) +def _validate_flows_structure(validator: "Validator") -> bool: + return validator.verify_flows_structure() + + def cancel_cause_not_found( current: Optional[Union["Path", Text]], parameter: Text, diff --git a/rasa/shared/core/flows/flow.py b/rasa/shared/core/flows/flow.py index c61a81d47288..47db88adfd26 100644 --- a/rasa/shared/core/flows/flow.py +++ b/rasa/shared/core/flows/flow.py @@ -38,7 +38,7 @@ def __str__(self) -> Text: class UnresolvedFlowStepIdException(RasaException): - """Raised when a flow step is referenced but it's id can not be resolved.""" + """Raised when a flow step is referenced, but its id can not be resolved.""" def __init__( self, step_id: Text, flow: Flow, referenced_from: Optional[FlowStep] diff --git a/rasa/validator.py b/rasa/validator.py index c2d7fed6742e..2d1ed68c3f53 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -1,12 +1,18 @@ import logging +import string from collections import defaultdict from typing import Set, Text, Optional, Dict, Any, List +from pypred import Predicate + import rasa.core.training.story_conflict from rasa.shared.core.flows.flow import ( ActionFlowStep, + BranchFlowStep, CollectInformationFlowStep, FlowsList, + IfFlowLink, + SetSlotsFlowStep, ) import rasa.shared.nlu.constants from rasa.shared.constants import ( @@ -28,6 +34,7 @@ from rasa.shared.core.generator import TrainingDataGenerator from rasa.shared.core.constants import SlotMappingType, MAPPING_TYPE from rasa.shared.core.training_data.structures import StoryGraph +from rasa.shared.exceptions import RasaException from rasa.shared.importers.importer import TrainingDataImporter from rasa.shared.nlu.training_data.training_data import TrainingData import rasa.shared.utils.io @@ -483,3 +490,132 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: f"'{ASSISTANT_ID_KEY}' mandatory key. Please replace the default " f"placeholder value with a unique identifier." ) + + def verify_flows_steps_against_domain(self) -> bool: + """Checks flows steps' references against the domain file.""" + all_good = True + domain_slot_names = [slot.name for slot in self.domain.slots] + for flow in self.flows.underlying_flows: + for step in flow.steps: + if isinstance(step, CollectInformationFlowStep): + if step.collect_information not in domain_slot_names: + raise RasaException( + f"The slot '{step.collect_information}' is used in the " + f"step '{step.id}' of flow '{flow.name}', but it " + f"is not listed in the domain slots. " + f"You should add it to your domain file!", + ) + + elif isinstance(step, SetSlotsFlowStep): + for slot in step.slots: + slot_name = slot["key"] + if slot_name not in domain_slot_names: + raise RasaException( + f"The slot '{slot_name}' is used in the step " + f"'{step.id}' of flow '{flow.name}', but it " + f"is not listed in the domain slots. " + f"You should add it to your domain file!", + ) + + elif isinstance(step, ActionFlowStep): + if step.action not in self.domain.action_names_or_texts: + raise RasaException( + f"The action '{step.action}' is used in the step " + f"'{step.id}' of flow '{flow.name}', but it " + f"is not listed in the domain file. " + f"You should add it to your domain file!", + ) + return all_good + + def verify_unique_flows(self) -> bool: + """Checks if all flows have unique names and descriptions.""" + all_good = True + + flows_mapping: Dict[str, str] = {} + punctuation_table = str.maketrans({i: "" for i in string.punctuation}) + + for flow in self.flows.underlying_flows: + flow_description = flow.description + cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 + if cleaned_description in flows_mapping.values(): + raise RasaException( + f"Detected duplicate flow description for flow '{flow.name}'. " + f"Flow descriptions must be unique. " + f"Please make sure that all flows have different descriptions." + ) + + if flow.name in flows_mapping: + raise RasaException( + f"Detected duplicate flow name '{flow.name}'. " + f"Flow names must be unique. " + f"Please make sure that all flows have different names." + ) + + flows_mapping[flow.name] = cleaned_description + + return all_good + + def verify_predicates(self) -> bool: + """Checks that predicates used in branch flow steps or `collect_information` steps are valid.""" # noqa: E501 + all_good = True + for flow in self.flows.underlying_flows: + for step in flow.steps: + if isinstance(step, BranchFlowStep): + for link in step.next.links: + if isinstance(link, IfFlowLink): + try: + predicate = Predicate(link.condition) + except (TypeError, Exception) as exception: + raise RasaException( + f"Could not initialize the predicate found " + f"under step '{step.id}'. Please make sure " + f"that all predicates are strings." + ) from exception + + is_valid = predicate.is_valid() + if not is_valid: + raise RasaException( + f"Detected invalid condition '{link.condition}' " + f"at step '{step.id}' for flow '{flow.name}'. " + f"Please make sure that all conditions are valid." + ) + elif isinstance(step, CollectInformationFlowStep): + predicates = [predicate.if_ for predicate in step.rejections] + for predicate in predicates: + try: + pred = Predicate(predicate) + except (TypeError, Exception) as exception: + raise RasaException( + f"Could not initialize the predicate found under step " + f"'{step.id}'. Please make sure that all predicates " + f"are strings." + ) from exception + + is_valid = pred.is_valid() + if not is_valid: + raise RasaException( + f"Detected invalid rejection '{predicate}' " + f"at `collect_information` step '{step.id}' " + f"for flow '{flow.name}'. " + f"Please make sure that all conditions are valid." + ) + return all_good + + def verify_flows_structure(self) -> bool: + """Checks if the flows structure is valid.""" + if self.flows.is_empty(): + rasa.shared.utils.io.raise_warning( + "No flows were found in the data files." + "Will not proceed with flow validation.", + ) + return True + + self.flows.validate() + + all_good = ( + self.verify_flows_steps_against_domain() + and self.verify_unique_flows() + and self.verify_predicates() + ) + + return all_good diff --git a/tests/test_validator.py b/tests/test_validator.py index d901d73fb333..03e0656345a4 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -5,6 +5,7 @@ import pytest from _pytest.logging import LogCaptureFixture from rasa.shared.constants import LATEST_TRAINING_DATA_FORMAT_VERSION +from rasa.shared.exceptions import RasaException from rasa.validator import Validator @@ -856,3 +857,397 @@ def test_warn_if_config_mandatory_keys_are_not_set_invalid_paths( with pytest.warns(UserWarning, match=message): validator.warn_if_config_mandatory_keys_are_not_set() + + +def test_verify_flow_steps_against_domain_missing_slot_in_domain( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + missing_slot_in_domain = "transfer_amount" + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: This flow lets users send money. + name: transfer money + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: {missing_slot_in_domain} + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + actions: + - action_transfer_money + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain() + + assert ( + f"The slot '{missing_slot_in_domain}' is used in the step 'ask_amount' of " + f"flow 'transfer money', but it is not listed in the domain slots." + ) in str(e.value) + + +def test_verify_flow_steps_against_domain_missing_action_in_domain( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + missing_action_in_domain = "action_transfer_money" + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: This flow lets users send money. + name: transfer money + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "execute_transfer" + - id: "execute_transfer" + action: {missing_action_in_domain} + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: text + mappings: [] + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain() + + assert ( + f"The action '{missing_action_in_domain}' is used in the step " + f"'execute_transfer' of flow 'transfer money', but it " + f"is not listed in the domain file." + ) in str(e.value) + + +def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + missing_slot_in_domain = "account_type" + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: This flow lets users send money. + name: transfer money + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "set_account_type" + - id: "set_account_type" + set_slots: + - {missing_slot_in_domain}: "debit" + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: text + mappings: [] + actions: + - action_transfer_money + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain() + + assert ( + f"The slot '{missing_slot_in_domain}' is used in the step " + f"'set_account_type' of flow 'transfer money', " + f"but it is not listed in the domain slots." + ) in str(e.value) + + +def test_verify_unique_flows_duplicate_names( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + duplicate_flow_name = "transfer money" + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: This flow lets users send money. + name: {duplicate_flow_name} + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money + recurrent_payment: + description: This flow sets up a recurrent payment. + name: {duplicate_flow_name} + steps: + - id: "set_up_recurrence" + action: action_set_up_recurrent_payment + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: text + mappings: [] + actions: + - action_transfer_money + - action_set_up_recurrent_payment + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_unique_flows() + + assert ( + f"Detected duplicate flow name '{duplicate_flow_name}'. " + f"Flow names must be unique. " + f"Please make sure that all flows have different names." + ) in str(e.value) + + +def test_verify_unique_flows_duplicate_descriptions( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + duplicate_flow_description_with_punctuation = "This flow lets users send money." + duplicate_flow_description = "This flow lets users send money" + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: {duplicate_flow_description_with_punctuation} + name: transfer money + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money + recurrent_payment: + description: {duplicate_flow_description} + name: setup recurrent payment + steps: + - id: "set_up_recurrence" + action: action_set_up_recurrent_payment + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: text + mappings: [] + actions: + - action_transfer_money + - action_set_up_recurrent_payment + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_unique_flows() + + assert ( + "Detected duplicate flow description for flow 'setup recurrent payment'. " + "Flow descriptions must be unique. " + "Please make sure that all flows have different descriptions." + ) in str(e.value) + + +def test_verify_predicates_invalid_rejection_if( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + predicate = 'account_type not in {{"debit", "savings"}}' + expected_exception = ( + f"Detected invalid rejection '{predicate}' " + f"at `collect_information` step 'ask_account_type' " + f"for flow 'transfer money'. " + f"Please make sure that all conditions are valid." + ) + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + transfer_money: + description: This flow lets users send money. + name: transfer money + steps: + - id: "ask_account_type" + collect_information: account_type + rejections: + - if: {predicate} + utter: utter_invalid_account_type + next: "ask_recipient" + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money + recurrent_payment: + description: This flow setups recurrent payments + name: setup recurrent payment + steps: + - id: "set_up_recurrence" + action: action_set_up_recurrent_payment + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: text + mappings: [] + actions: + - action_transfer_money + - action_set_up_recurrent_payment + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with pytest.raises(RasaException) as e: + validator.verify_predicates() + + assert expected_exception in str(e.value) From 56bd648dd05bee2feff54c52a73478492058647b Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Mon, 25 Sep 2023 17:14:14 +0100 Subject: [PATCH 02/10] replace user warning with logging, fix cli test --- rasa/validator.py | 4 ++-- tests/cli/test_rasa_data.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index 2d1ed68c3f53..25f92345b98a 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -604,8 +604,8 @@ def verify_predicates(self) -> bool: def verify_flows_structure(self) -> bool: """Checks if the flows structure is valid.""" if self.flows.is_empty(): - rasa.shared.utils.io.raise_warning( - "No flows were found in the data files." + logger.warning( + "No flows were found in the data files. " "Will not proceed with flow validation.", ) return True diff --git a/tests/cli/test_rasa_data.py b/tests/cli/test_rasa_data.py index e8c2f869b9ca..95b109852049 100644 --- a/tests/cli/test_rasa_data.py +++ b/tests/cli/test_rasa_data.py @@ -139,7 +139,7 @@ def test_data_validate_help(run: Callable[..., RunResult]): [--max-history MAX_HISTORY] [-c CONFIG] [--fail-on-warnings] [-d DOMAIN] [--data DATA [DATA ...]] - {{stories}} ...""" + {{stories,flows}} ...""" lines = help_text.split("\n") # expected help text lines should appear somewhere in the output From a89ec179a1716712deafe9cc0dc8ff3d97e67c80 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Tue, 26 Sep 2023 10:52:03 +0100 Subject: [PATCH 03/10] add cli test, apply validation checks only to user defined flows --- rasa/cli/utils.py | 8 +++--- rasa/validator.py | 31 +++++++++++++--------- tests/cli/test_rasa_data.py | 40 ++++++++++++++++++++++++++++ tests/test_validator.py | 52 +++++++++++++++++++++++++++++-------- 4 files changed, 104 insertions(+), 27 deletions(-) diff --git a/rasa/cli/utils.py b/rasa/cli/utils.py index c92f38cfe9e5..0e7edc30f4dd 100644 --- a/rasa/cli/utils.py +++ b/rasa/cli/utils.py @@ -235,7 +235,7 @@ def validate_files( if stories_only: all_good = _validate_story_structure(validator, max_history, fail_on_warnings) elif flows_only: - all_good = _validate_flows_structure(validator) + all_good = _validate_flows(validator) else: if importer.get_domain().is_empty(): rasa.shared.utils.cli.print_error_and_exit( @@ -247,7 +247,7 @@ def validate_files( valid_stories = _validate_story_structure( validator, max_history, fail_on_warnings ) - valid_flows = _validate_flows_structure(validator) + valid_flows = _validate_flows(validator) all_good = valid_domain and valid_nlu and valid_stories and valid_flows @@ -293,8 +293,8 @@ def _validate_story_structure( ) -def _validate_flows_structure(validator: "Validator") -> bool: - return validator.verify_flows_structure() +def _validate_flows(validator: "Validator") -> bool: + return validator.verify_flows() def cancel_cause_not_found( diff --git a/rasa/validator.py b/rasa/validator.py index 25f92345b98a..b2d85256bf70 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -10,6 +10,7 @@ ActionFlowStep, BranchFlowStep, CollectInformationFlowStep, + Flow, FlowsList, IfFlowLink, SetSlotsFlowStep, @@ -491,11 +492,11 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: f"placeholder value with a unique identifier." ) - def verify_flows_steps_against_domain(self) -> bool: + def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: """Checks flows steps' references against the domain file.""" all_good = True domain_slot_names = [slot.name for slot in self.domain.slots] - for flow in self.flows.underlying_flows: + for flow in user_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): if step.collect_information not in domain_slot_names: @@ -527,14 +528,15 @@ def verify_flows_steps_against_domain(self) -> bool: ) return all_good - def verify_unique_flows(self) -> bool: + @staticmethod + def verify_unique_flows(user_flows: List[Flow]) -> bool: """Checks if all flows have unique names and descriptions.""" all_good = True flows_mapping: Dict[str, str] = {} punctuation_table = str.maketrans({i: "" for i in string.punctuation}) - for flow in self.flows.underlying_flows: + for flow in user_flows: flow_description = flow.description cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 if cleaned_description in flows_mapping.values(): @@ -555,10 +557,11 @@ def verify_unique_flows(self) -> bool: return all_good - def verify_predicates(self) -> bool: + @staticmethod + def verify_predicates(user_flows: List[Flow]) -> bool: """Checks that predicates used in branch flow steps or `collect_information` steps are valid.""" # noqa: E501 all_good = True - for flow in self.flows.underlying_flows: + for flow in user_flows: for step in flow.steps: if isinstance(step, BranchFlowStep): for link in step.next.links: @@ -601,8 +604,8 @@ def verify_predicates(self) -> bool: ) return all_good - def verify_flows_structure(self) -> bool: - """Checks if the flows structure is valid.""" + def verify_flows(self) -> bool: + """Checks for inconsistencies across flows.""" if self.flows.is_empty(): logger.warning( "No flows were found in the data files. " @@ -610,12 +613,16 @@ def verify_flows_structure(self) -> bool: ) return True - self.flows.validate() + user_flows = [ + flow + for flow in self.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] all_good = ( - self.verify_flows_steps_against_domain() - and self.verify_unique_flows() - and self.verify_predicates() + self.verify_flows_steps_against_domain(user_flows) + and self.verify_unique_flows(user_flows) + and self.verify_predicates(user_flows) ) return all_good diff --git a/tests/cli/test_rasa_data.py b/tests/cli/test_rasa_data.py index 95b109852049..f8c4dc674362 100644 --- a/tests/cli/test_rasa_data.py +++ b/tests/cli/test_rasa_data.py @@ -4,6 +4,8 @@ from _pytest.fixtures import FixtureRequest from _pytest.pytester import RunResult + +from rasa.shared.constants import LATEST_TRAINING_DATA_FORMAT_VERSION from rasa.shared.nlu.training_data.formats import RasaYAMLReader import rasa.shared.utils.io @@ -255,3 +257,41 @@ def test_data_split_stories(run_in_simple_project: Callable[..., RunResult]): test_data = rasa.shared.utils.io.read_yaml_file(test_file) assert len(test_data.get("stories", [])) == 1 assert test_data["stories"][0].get("story") == "story 2" + + +def test_rasa_data_validate_flows_success( + run_in_simple_project: Callable[..., RunResult] +) -> None: + flows_yaml = f""" +version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" +flows: + transfer_money: + description: This flow lets users send money. + name: transfer money + steps: + - id: "ask_recipient" + collect_information: transfer_recipient + next: "ask_amount" + - id: "ask_amount" + collect_information: transfer_amount + next: "execute_transfer" + - id: "execute_transfer" + action: action_transfer_money""" + + Path("data/flows.yml").write_text(flows_yaml) + + domain_yaml = """ + actions: + - action_transfer_money + intents: + - transfer_money + slots: + transfer_recipient: + type: text + mappings: [] + transfer_amount: + type: float + mappings: []""" + Path("domain.yml").write_text(domain_yaml) + result = run_in_simple_project("data", "validate", "flows") + assert result.ret == 0 diff --git a/tests/test_validator.py b/tests/test_validator.py index 03e0656345a4..da7462612789 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -906,9 +906,14 @@ def test_verify_flow_steps_against_domain_missing_slot_in_domain( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + validator.verify_flows_steps_against_domain(user_flows) assert ( f"The slot '{missing_slot_in_domain}' is used in the step 'ask_amount' of " @@ -953,7 +958,7 @@ def test_verify_flow_steps_against_domain_missing_action_in_domain( type: text mappings: [] transfer_amount: - type: text + type: float mappings: [] """ ) @@ -964,9 +969,14 @@ def test_verify_flow_steps_against_domain_missing_action_in_domain( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + validator.verify_flows_steps_against_domain(user_flows) assert ( f"The action '{missing_action_in_domain}' is used in the step " @@ -1016,7 +1026,7 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( type: text mappings: [] transfer_amount: - type: text + type: float mappings: [] actions: - action_transfer_money @@ -1029,9 +1039,14 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + validator.verify_flows_steps_against_domain(user_flows) assert ( f"The slot '{missing_slot_in_domain}' is used in the step " @@ -1083,7 +1098,7 @@ def test_verify_unique_flows_duplicate_names( type: text mappings: [] transfer_amount: - type: text + type: float mappings: [] actions: - action_transfer_money @@ -1097,9 +1112,14 @@ def test_verify_unique_flows_duplicate_names( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_unique_flows() + validator.verify_unique_flows(user_flows) assert ( f"Detected duplicate flow name '{duplicate_flow_name}'. " @@ -1152,7 +1172,7 @@ def test_verify_unique_flows_duplicate_descriptions( type: text mappings: [] transfer_amount: - type: text + type: float mappings: [] actions: - action_transfer_money @@ -1166,9 +1186,14 @@ def test_verify_unique_flows_duplicate_descriptions( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_unique_flows() + validator.verify_unique_flows(user_flows) assert ( "Detected duplicate flow description for flow 'setup recurrent payment'. " @@ -1232,7 +1257,7 @@ def test_verify_predicates_invalid_rejection_if( type: text mappings: [] transfer_amount: - type: text + type: float mappings: [] actions: - action_transfer_money @@ -1246,8 +1271,13 @@ def test_verify_predicates_invalid_rejection_if( ) validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] with pytest.raises(RasaException) as e: - validator.verify_predicates() + validator.verify_predicates(user_flows) assert expected_exception in str(e.value) From 1042d5bef0d6b827ea1f2425fe168e8cb3d33d19 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Tue, 26 Sep 2023 14:41:58 +0100 Subject: [PATCH 04/10] disallow list slots in flows, add log, amend exception messages --- rasa/validator.py | 43 ++++++++++++++++++------ tests/test_validator.py | 74 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 99 insertions(+), 18 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index b2d85256bf70..cd2a49c4ed58 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -34,6 +34,7 @@ from rasa.shared.core.domain import Domain from rasa.shared.core.generator import TrainingDataGenerator from rasa.shared.core.constants import SlotMappingType, MAPPING_TYPE +from rasa.shared.core.slots import ListSlot from rasa.shared.core.training_data.structures import StoryGraph from rasa.shared.exceptions import RasaException from rasa.shared.importers.importer import TrainingDataImporter @@ -495,34 +496,52 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: """Checks flows steps' references against the domain file.""" all_good = True - domain_slot_names = [slot.name for slot in self.domain.slots] + domain_slots = {slot.name: slot for slot in self.domain.slots} for flow in user_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): - if step.collect_information not in domain_slot_names: + if step.collect_information not in domain_slots: raise RasaException( f"The slot '{step.collect_information}' is used in the " - f"step '{step.id}' of flow '{flow.name}', but it " + f"step '{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain slots. " f"You should add it to your domain file!", ) + current_slot = domain_slots[step.collect_information] + if isinstance(current_slot, ListSlot): + raise RasaException( + f"The slot '{step.collect_information}' is used in the " + f"step '{step.id}' of flow id '{flow.id}', but it " + f"is a list slot. List slots are currently not " + f"supported in flows. You should change it to a " + f"text, boolean or float slot in your domain file!", + ) elif isinstance(step, SetSlotsFlowStep): for slot in step.slots: slot_name = slot["key"] - if slot_name not in domain_slot_names: + if slot_name not in domain_slots: raise RasaException( f"The slot '{slot_name}' is used in the step " - f"'{step.id}' of flow '{flow.name}', but it " + f"'{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain slots. " f"You should add it to your domain file!", ) + current_slot = domain_slots[slot_name] + if isinstance(current_slot, ListSlot): + raise RasaException( + f"The slot '{slot_name}' is used in the " + f"step '{step.id}' of flow id '{flow.id}', but it " + f"is a list slot. List slots are currently not " + f"supported in flows. You should change it to a " + f"text, boolean or float slot in your domain file!", + ) elif isinstance(step, ActionFlowStep): if step.action not in self.domain.action_names_or_texts: raise RasaException( f"The action '{step.action}' is used in the step " - f"'{step.id}' of flow '{flow.name}', but it " + f"'{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain file. " f"You should add it to your domain file!", ) @@ -541,15 +560,15 @@ def verify_unique_flows(user_flows: List[Flow]) -> bool: cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 if cleaned_description in flows_mapping.values(): raise RasaException( - f"Detected duplicate flow description for flow '{flow.name}'. " + f"Detected duplicate flow description for flow id '{flow.id}'. " f"Flow descriptions must be unique. " f"Please make sure that all flows have different descriptions." ) if flow.name in flows_mapping: raise RasaException( - f"Detected duplicate flow name '{flow.name}'. " - f"Flow names must be unique. " + f"Detected duplicate flow name '{flow.name}' for flow " + f"id '{flow.id}'. Flow names must be unique. " f"Please make sure that all flows have different names." ) @@ -579,7 +598,7 @@ def verify_predicates(user_flows: List[Flow]) -> bool: if not is_valid: raise RasaException( f"Detected invalid condition '{link.condition}' " - f"at step '{step.id}' for flow '{flow.name}'. " + f"at step '{step.id}' for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) elif isinstance(step, CollectInformationFlowStep): @@ -599,13 +618,15 @@ def verify_predicates(user_flows: List[Flow]) -> bool: raise RasaException( f"Detected invalid rejection '{predicate}' " f"at `collect_information` step '{step.id}' " - f"for flow '{flow.name}'. " + f"for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) return all_good def verify_flows(self) -> bool: """Checks for inconsistencies across flows.""" + logger.info("Validating flows...") + if self.flows.is_empty(): logger.warning( "No flows were found in the data files. " diff --git a/tests/test_validator.py b/tests/test_validator.py index da7462612789..a193416a0557 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -917,7 +917,7 @@ def test_verify_flow_steps_against_domain_missing_slot_in_domain( assert ( f"The slot '{missing_slot_in_domain}' is used in the step 'ask_amount' of " - f"flow 'transfer money', but it is not listed in the domain slots." + f"flow id 'transfer_money', but it is not listed in the domain slots." ) in str(e.value) @@ -980,7 +980,7 @@ def test_verify_flow_steps_against_domain_missing_action_in_domain( assert ( f"The action '{missing_action_in_domain}' is used in the step " - f"'execute_transfer' of flow 'transfer money', but it " + f"'execute_transfer' of flow id 'transfer_money', but it " f"is not listed in the domain file." ) in str(e.value) @@ -1050,11 +1050,71 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( assert ( f"The slot '{missing_slot_in_domain}' is used in the step " - f"'set_account_type' of flow 'transfer money', " + f"'set_account_type' of flow id 'transfer_money', " f"but it is not listed in the domain slots." ) in str(e.value) +def test_verify_flow_steps_against_domain_disallowed_list_slot( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + order_pizza: + description: This flow lets users order their favourite pizza. + name: order pizza + steps: + - id: "ask_pizza_toppings" + collect_information: pizza_toppings + next: "ask_address" + - id: "ask_address" + collect_information: address + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + slots: + pizza_toppings: + type: list + mappings: [] + address: + type: text + mappings: [] + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain(user_flows) + + assert ( + "The slot 'pizza_toppings' is used in the step 'ask_pizza_toppings' " + "of flow id 'order_pizza', but it is a list slot. List slots are " + "currently not supported in flows." in str(e.value) + ) + + def test_verify_unique_flows_duplicate_names( tmp_path: Path, nlu_data_path: Path, @@ -1122,8 +1182,8 @@ def test_verify_unique_flows_duplicate_names( validator.verify_unique_flows(user_flows) assert ( - f"Detected duplicate flow name '{duplicate_flow_name}'. " - f"Flow names must be unique. " + f"Detected duplicate flow name '{duplicate_flow_name}' for " + f"flow id 'recurrent_payment'. Flow names must be unique. " f"Please make sure that all flows have different names." ) in str(e.value) @@ -1196,7 +1256,7 @@ def test_verify_unique_flows_duplicate_descriptions( validator.verify_unique_flows(user_flows) assert ( - "Detected duplicate flow description for flow 'setup recurrent payment'. " + "Detected duplicate flow description for flow id 'recurrent_payment'. " "Flow descriptions must be unique. " "Please make sure that all flows have different descriptions." ) in str(e.value) @@ -1210,7 +1270,7 @@ def test_verify_predicates_invalid_rejection_if( expected_exception = ( f"Detected invalid rejection '{predicate}' " f"at `collect_information` step 'ask_account_type' " - f"for flow 'transfer money'. " + f"for flow id 'transfer_money'. " f"Please make sure that all conditions are valid." ) flows_file = tmp_path / "flows.yml" From 008a9c2457bbb938137b67eecda022d595eaa77c Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Tue, 26 Sep 2023 15:39:00 +0100 Subject: [PATCH 05/10] raise exception if internal dialogue stack slot is used in flows --- rasa/validator.py | 51 +++++++++++++++++++++++++++-------------- tests/test_validator.py | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 17 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index cd2a49c4ed58..26701c53c41b 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -34,7 +34,7 @@ from rasa.shared.core.domain import Domain from rasa.shared.core.generator import TrainingDataGenerator from rasa.shared.core.constants import SlotMappingType, MAPPING_TYPE -from rasa.shared.core.slots import ListSlot +from rasa.shared.core.slots import ListSlot, Slot from rasa.shared.core.training_data.structures import StoryGraph from rasa.shared.exceptions import RasaException from rasa.shared.importers.importer import TrainingDataImporter @@ -493,6 +493,29 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: f"placeholder value with a unique identifier." ) + @staticmethod + def _raise_exception_if_list_slot(slot: Slot, step_id: str, flow_id: str) -> None: + if isinstance(slot, ListSlot): + raise RasaException( + f"The slot '{slot.name}' is used in the " + f"step '{step_id}' of flow id '{flow_id}', but it " + f"is a list slot. List slots are currently not " + f"supported in flows. You should change it to a " + f"text, boolean or float slot in your domain file!", + ) + + @staticmethod + def _raise_exception_if_dialogue_stack_slot( + slot: Slot, step_id: str, flow_id: str + ) -> None: + if slot.name == constants.DIALOGUE_STACK_SLOT: + raise RasaException( + f"The slot '{constants.DIALOGUE_STACK_SLOT}' is used in the " + f"step '{step_id}' of flow id '{flow_id}', but it " + f"is a reserved slot. You must not use reserved slots in " + f"your flows.", + ) + def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: """Checks flows steps' references against the domain file.""" all_good = True @@ -508,14 +531,10 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: f"You should add it to your domain file!", ) current_slot = domain_slots[step.collect_information] - if isinstance(current_slot, ListSlot): - raise RasaException( - f"The slot '{step.collect_information}' is used in the " - f"step '{step.id}' of flow id '{flow.id}', but it " - f"is a list slot. List slots are currently not " - f"supported in flows. You should change it to a " - f"text, boolean or float slot in your domain file!", - ) + self._raise_exception_if_list_slot(current_slot, step.id, flow.id) + self._raise_exception_if_dialogue_stack_slot( + current_slot, step.id, flow.id + ) elif isinstance(step, SetSlotsFlowStep): for slot in step.slots: @@ -528,14 +547,12 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: f"You should add it to your domain file!", ) current_slot = domain_slots[slot_name] - if isinstance(current_slot, ListSlot): - raise RasaException( - f"The slot '{slot_name}' is used in the " - f"step '{step.id}' of flow id '{flow.id}', but it " - f"is a list slot. List slots are currently not " - f"supported in flows. You should change it to a " - f"text, boolean or float slot in your domain file!", - ) + self._raise_exception_if_list_slot( + current_slot, step.id, flow.id + ) + self._raise_exception_if_dialogue_stack_slot( + current_slot, step.id, flow.id + ) elif isinstance(step, ActionFlowStep): if step.action not in self.domain.action_names_or_texts: diff --git a/tests/test_validator.py b/tests/test_validator.py index a193416a0557..3216a1715b00 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -1115,6 +1115,55 @@ def test_verify_flow_steps_against_domain_disallowed_list_slot( ) +def test_verify_flow_steps_against_domain_dialogue_stack_slot( + tmp_path: Path, + nlu_data_path: Path, +) -> None: + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + my_flow: + description: Test that dialogue stack is not modified in flows. + name: test flow + steps: + - id: "ask_internal_slot" + collect_information: dialogue_stack + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + user_flows = [ + flow + for flow in validator.flows.underlying_flows + if not flow.id.startswith("pattern_") + ] + + with pytest.raises(RasaException) as e: + validator.verify_flows_steps_against_domain(user_flows) + + assert ( + "The slot 'dialogue_stack' is used in the step 'ask_internal_slot' " + "of flow id 'my_flow', but it is a reserved slot." in str(e.value) + ) + + def test_verify_unique_flows_duplicate_names( tmp_path: Path, nlu_data_path: Path, From a1714b0e6c6e405a6a5f4bfde8419e8e93dc1a95 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Wed, 27 Sep 2023 11:22:25 +0100 Subject: [PATCH 06/10] address review comments, some refactoring --- rasa/cli/utils.py | 8 +- rasa/validator.py | 82 +++++++++--------- tests/test_validator.py | 184 +++++++++------------------------------- 3 files changed, 81 insertions(+), 193 deletions(-) diff --git a/rasa/cli/utils.py b/rasa/cli/utils.py index 0e7edc30f4dd..6a2083c8982b 100644 --- a/rasa/cli/utils.py +++ b/rasa/cli/utils.py @@ -235,7 +235,7 @@ def validate_files( if stories_only: all_good = _validate_story_structure(validator, max_history, fail_on_warnings) elif flows_only: - all_good = _validate_flows(validator) + all_good = validator.verify_flows() else: if importer.get_domain().is_empty(): rasa.shared.utils.cli.print_error_and_exit( @@ -247,7 +247,7 @@ def validate_files( valid_stories = _validate_story_structure( validator, max_history, fail_on_warnings ) - valid_flows = _validate_flows(validator) + valid_flows = validator.verify_flows() all_good = valid_domain and valid_nlu and valid_stories and valid_flows @@ -293,10 +293,6 @@ def _validate_story_structure( ) -def _validate_flows(validator: "Validator") -> bool: - return validator.verify_flows() - - def cancel_cause_not_found( current: Optional[Union["Path", Text]], parameter: Text, diff --git a/rasa/validator.py b/rasa/validator.py index 26701c53c41b..6e2893472cf3 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -493,6 +493,18 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: f"placeholder value with a unique identifier." ) + @staticmethod + def _raise_exception_if_slot_not_in_domain( + slot_name: str, domain_slots: Dict[Text, Slot], step_id: str, flow_id: str + ) -> None: + if slot_name not in domain_slots: + raise RasaException( + f"The slot '{slot_name}' is used in the " + f"step '{step_id}' of flow id '{flow_id}', but it " + f"is not listed in the domain slots. " + f"You should add it to your domain file!", + ) + @staticmethod def _raise_exception_if_list_slot(slot: Slot, step_id: str, flow_id: str) -> None: if isinstance(slot, ListSlot): @@ -518,18 +530,14 @@ def _raise_exception_if_dialogue_stack_slot( def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: """Checks flows steps' references against the domain file.""" - all_good = True domain_slots = {slot.name: slot for slot in self.domain.slots} for flow in user_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): - if step.collect_information not in domain_slots: - raise RasaException( - f"The slot '{step.collect_information}' is used in the " - f"step '{step.id}' of flow id '{flow.id}', but it " - f"is not listed in the domain slots. " - f"You should add it to your domain file!", - ) + self._raise_exception_if_slot_not_in_domain( + step.collect_information, domain_slots, step.id, flow.id + ) + current_slot = domain_slots[step.collect_information] self._raise_exception_if_list_slot(current_slot, step.id, flow.id) self._raise_exception_if_dialogue_stack_slot( @@ -539,13 +547,10 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: elif isinstance(step, SetSlotsFlowStep): for slot in step.slots: slot_name = slot["key"] - if slot_name not in domain_slots: - raise RasaException( - f"The slot '{slot_name}' is used in the step " - f"'{step.id}' of flow id '{flow.id}', but it " - f"is not listed in the domain slots. " - f"You should add it to your domain file!", - ) + self._raise_exception_if_slot_not_in_domain( + slot_name, domain_slots, step.id, flow.id + ) + current_slot = domain_slots[slot_name] self._raise_exception_if_list_slot( current_slot, step.id, flow.id @@ -562,13 +567,11 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: f"is not listed in the domain file. " f"You should add it to your domain file!", ) - return all_good + return True @staticmethod def verify_unique_flows(user_flows: List[Flow]) -> bool: """Checks if all flows have unique names and descriptions.""" - all_good = True - flows_mapping: Dict[str, str] = {} punctuation_table = str.maketrans({i: "" for i in string.punctuation}) @@ -591,7 +594,20 @@ def verify_unique_flows(user_flows: List[Flow]) -> bool: flows_mapping[flow.name] = cleaned_description - return all_good + return True + + @staticmethod + def _construct_predicate(predicate: Optional[str], step_id: str) -> Predicate: + try: + pred = Predicate(predicate) + except (TypeError, Exception) as exception: + raise RasaException( + f"Could not initialize the predicate found under step " + f"'{step_id}'. Please make sure that all predicates " + f"are strings." + ) from exception + + return pred @staticmethod def verify_predicates(user_flows: List[Flow]) -> bool: @@ -602,17 +618,10 @@ def verify_predicates(user_flows: List[Flow]) -> bool: if isinstance(step, BranchFlowStep): for link in step.next.links: if isinstance(link, IfFlowLink): - try: - predicate = Predicate(link.condition) - except (TypeError, Exception) as exception: - raise RasaException( - f"Could not initialize the predicate found " - f"under step '{step.id}'. Please make sure " - f"that all predicates are strings." - ) from exception - - is_valid = predicate.is_valid() - if not is_valid: + predicate = Validator._construct_predicate( + link.condition, step.id + ) + if not predicate.is_valid(): raise RasaException( f"Detected invalid condition '{link.condition}' " f"at step '{step.id}' for flow id '{flow.id}'. " @@ -621,17 +630,8 @@ def verify_predicates(user_flows: List[Flow]) -> bool: elif isinstance(step, CollectInformationFlowStep): predicates = [predicate.if_ for predicate in step.rejections] for predicate in predicates: - try: - pred = Predicate(predicate) - except (TypeError, Exception) as exception: - raise RasaException( - f"Could not initialize the predicate found under step " - f"'{step.id}'. Please make sure that all predicates " - f"are strings." - ) from exception - - is_valid = pred.is_valid() - if not is_valid: + pred = Validator._construct_predicate(predicate, step.id) + if not pred.is_valid(): raise RasaException( f"Detected invalid rejection '{predicate}' " f"at `collect_information` step '{step.id}' " diff --git a/tests/test_validator.py b/tests/test_validator.py index 3216a1715b00..1abd583c550e 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -1,6 +1,6 @@ import textwrap import warnings -from typing import Text +from typing import Any, Dict, List, Text import pytest from _pytest.logging import LogCaptureFixture @@ -859,137 +859,42 @@ def test_warn_if_config_mandatory_keys_are_not_set_invalid_paths( validator.warn_if_config_mandatory_keys_are_not_set() -def test_verify_flow_steps_against_domain_missing_slot_in_domain( - tmp_path: Path, - nlu_data_path: Path, -) -> None: - missing_slot_in_domain = "transfer_amount" - flows_file = tmp_path / "flows.yml" - with open(flows_file, "w") as file: - file.write( - f""" - version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" - flows: - transfer_money: - description: This flow lets users send money. - name: transfer money - steps: - - id: "ask_recipient" - collect_information: transfer_recipient - next: "ask_amount" - - id: "ask_amount" - collect_information: {missing_slot_in_domain} - next: "execute_transfer" - - id: "execute_transfer" - action: action_transfer_money - """ - ) - domain_file = tmp_path / "domain.yml" - with open(domain_file, "w") as file: - file.write( - f""" - version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" - intents: - - greet - slots: - transfer_recipient: - type: text - mappings: [] - actions: - - action_transfer_money - """ - ) - importer = RasaFileImporter( - config_file="data/test_moodbot/config.yml", - domain_path=str(domain_file), - training_data_paths=[str(flows_file), str(nlu_data_path)], - ) - - validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] - - with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain(user_flows) - - assert ( - f"The slot '{missing_slot_in_domain}' is used in the step 'ask_amount' of " - f"flow id 'transfer_money', but it is not listed in the domain slots." - ) in str(e.value) - - -def test_verify_flow_steps_against_domain_missing_action_in_domain( - tmp_path: Path, - nlu_data_path: Path, -) -> None: - missing_action_in_domain = "action_transfer_money" - flows_file = tmp_path / "flows.yml" - with open(flows_file, "w") as file: - file.write( - f""" - version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" - flows: - transfer_money: - description: This flow lets users send money. - name: transfer money - steps: - - id: "ask_recipient" - collect_information: transfer_recipient - next: "ask_amount" - - id: "ask_amount" - collect_information: transfer_amount - next: "execute_transfer" - - id: "execute_transfer" - action: {missing_action_in_domain} - """ - ) - domain_file = tmp_path / "domain.yml" - with open(domain_file, "w") as file: - file.write( - f""" - version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" - intents: - - greet - slots: - transfer_recipient: - type: text - mappings: [] - transfer_amount: - type: float - mappings: [] - """ - ) - importer = RasaFileImporter( - config_file="data/test_moodbot/config.yml", - domain_path=str(domain_file), - training_data_paths=[str(flows_file), str(nlu_data_path)], - ) - - validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] - - with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain(user_flows) - - assert ( - f"The action '{missing_action_in_domain}' is used in the step " - f"'execute_transfer' of flow id 'transfer_money', but it " - f"is not listed in the domain file." - ) in str(e.value) - - -def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( +@pytest.mark.parametrize( + "domain_actions, domain_slots, exception_message", + [ + # set_slot slot is not listed in the domain + ( + ["action_transfer_money"], + {"transfer_amount": {"type": "float", "mappings": []}}, + "The slot 'account_type' is used in the step 'set_account_type' " + "of flow id 'transfer_money', but it is not listed in the domain slots.", + ), + # collect_information slot is not listed in the domain + ( + ["action_transfer_money"], + {"account_type": {"type": "text", "mappings": []}}, + "The slot 'transfer_amount' is used in the step 'ask_amount' " + "of flow id 'transfer_money', but it is not listed in the domain slots.", + ), + # action name is not listed in the domain + ( + [], + { + "account_type": {"type": "text", "mappings": []}, + "transfer_amount": {"type": "float", "mappings": []}, + }, + "The action 'action_transfer_money' is used in the step 'execute_transfer' " + "of flow id 'transfer_money', but it is not listed in the domain file.", + ), + ], +) +def test_verify_flow_steps_against_domain_fail( tmp_path: Path, nlu_data_path: Path, + domain_actions: List[Text], + domain_slots: Dict[Text, Any], + exception_message: Text, ) -> None: - missing_slot_in_domain = "account_type" flows_file = tmp_path / "flows.yml" with open(flows_file, "w") as file: file.write( @@ -1000,15 +905,12 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( description: This flow lets users send money. name: transfer money steps: - - id: "ask_recipient" - collect_information: transfer_recipient - next: "ask_amount" - id: "ask_amount" collect_information: transfer_amount next: "set_account_type" - id: "set_account_type" set_slots: - - {missing_slot_in_domain}: "debit" + - account_type: "debit" next: "execute_transfer" - id: "execute_transfer" action: action_transfer_money @@ -1022,14 +924,8 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( intents: - greet slots: - transfer_recipient: - type: text - mappings: [] - transfer_amount: - type: float - mappings: [] - actions: - - action_transfer_money + {domain_slots} + actions: {domain_actions} """ ) importer = RasaFileImporter( @@ -1048,11 +944,7 @@ def test_verify_flow_steps_against_domain_missing_slot_from_set_slot_step( with pytest.raises(RasaException) as e: validator.verify_flows_steps_against_domain(user_flows) - assert ( - f"The slot '{missing_slot_in_domain}' is used in the step " - f"'set_account_type' of flow id 'transfer_money', " - f"but it is not listed in the domain slots." - ) in str(e.value) + assert exception_message in str(e.value) def test_verify_flow_steps_against_domain_disallowed_list_slot( From c1f854d0b7756e7f6f3c46bfcdde2b257fa6f163 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Wed, 27 Sep 2023 11:42:56 +0100 Subject: [PATCH 07/10] update collect_information occurences to collect --- rasa/validator.py | 8 ++++---- tests/test_validator.py | 26 +++++++++++++------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index 2afd89fe0d2f..edc3b44ba8d2 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -535,10 +535,10 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): self._raise_exception_if_slot_not_in_domain( - step.collect_information, domain_slots, step.id, flow.id + step.collect, domain_slots, step.id, flow.id ) - current_slot = domain_slots[step.collect_information] + current_slot = domain_slots[step.collect] self._raise_exception_if_list_slot(current_slot, step.id, flow.id) self._raise_exception_if_dialogue_stack_slot( current_slot, step.id, flow.id @@ -611,7 +611,7 @@ def _construct_predicate(predicate: Optional[str], step_id: str) -> Predicate: @staticmethod def verify_predicates(user_flows: List[Flow]) -> bool: - """Checks that predicates used in branch flow steps or `collect_information` steps are valid.""" # noqa: E501 + """Checks that predicates used in branch flow steps or `collect` steps are valid.""" # noqa: E501 all_good = True for flow in user_flows: for step in flow.steps: @@ -634,7 +634,7 @@ def verify_predicates(user_flows: List[Flow]) -> bool: if not pred.is_valid(): raise RasaException( f"Detected invalid rejection '{predicate}' " - f"at `collect_information` step '{step.id}' " + f"at `collect` step '{step.id}' " f"for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) diff --git a/tests/test_validator.py b/tests/test_validator.py index 1abd583c550e..385ba238f3e7 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -869,7 +869,7 @@ def test_warn_if_config_mandatory_keys_are_not_set_invalid_paths( "The slot 'account_type' is used in the step 'set_account_type' " "of flow id 'transfer_money', but it is not listed in the domain slots.", ), - # collect_information slot is not listed in the domain + # collect slot is not listed in the domain ( ["action_transfer_money"], {"account_type": {"type": "text", "mappings": []}}, @@ -906,7 +906,7 @@ def test_verify_flow_steps_against_domain_fail( name: transfer money steps: - id: "ask_amount" - collect_information: transfer_amount + collect: transfer_amount next: "set_account_type" - id: "set_account_type" set_slots: @@ -962,10 +962,10 @@ def test_verify_flow_steps_against_domain_disallowed_list_slot( name: order pizza steps: - id: "ask_pizza_toppings" - collect_information: pizza_toppings + collect: pizza_toppings next: "ask_address" - id: "ask_address" - collect_information: address + collect: address """ ) domain_file = tmp_path / "domain.yml" @@ -1022,7 +1022,7 @@ def test_verify_flow_steps_against_domain_dialogue_stack_slot( name: test flow steps: - id: "ask_internal_slot" - collect_information: dialogue_stack + collect: dialogue_stack """ ) domain_file = tmp_path / "domain.yml" @@ -1072,10 +1072,10 @@ def test_verify_unique_flows_duplicate_names( name: {duplicate_flow_name} steps: - id: "ask_recipient" - collect_information: transfer_recipient + collect: transfer_recipient next: "ask_amount" - id: "ask_amount" - collect_information: transfer_amount + collect: transfer_amount next: "execute_transfer" - id: "execute_transfer" action: action_transfer_money @@ -1146,10 +1146,10 @@ def test_verify_unique_flows_duplicate_descriptions( name: transfer money steps: - id: "ask_recipient" - collect_information: transfer_recipient + collect: transfer_recipient next: "ask_amount" - id: "ask_amount" - collect_information: transfer_amount + collect: transfer_amount next: "execute_transfer" - id: "execute_transfer" action: action_transfer_money @@ -1210,7 +1210,7 @@ def test_verify_predicates_invalid_rejection_if( predicate = 'account_type not in {{"debit", "savings"}}' expected_exception = ( f"Detected invalid rejection '{predicate}' " - f"at `collect_information` step 'ask_account_type' " + f"at `collect` step 'ask_account_type' " f"for flow id 'transfer_money'. " f"Please make sure that all conditions are valid." ) @@ -1225,16 +1225,16 @@ def test_verify_predicates_invalid_rejection_if( name: transfer money steps: - id: "ask_account_type" - collect_information: account_type + collect: account_type rejections: - if: {predicate} utter: utter_invalid_account_type next: "ask_recipient" - id: "ask_recipient" - collect_information: transfer_recipient + collect: transfer_recipient next: "ask_amount" - id: "ask_amount" - collect_information: transfer_amount + collect: transfer_amount next: "execute_transfer" - id: "execute_transfer" action: action_transfer_money From 12d6fe6c131ca4fbec14a43606fd8fed02425a83 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Wed, 27 Sep 2023 13:25:40 +0100 Subject: [PATCH 08/10] revert filtering of default flows --- .../patterns/default_flows_for_patterns.yml | 10 ++- rasa/validator.py | 40 +++++---- tests/test_validator.py | 88 +++++++++++-------- 3 files changed, 82 insertions(+), 56 deletions(-) diff --git a/rasa/dialogue_understanding/patterns/default_flows_for_patterns.yml b/rasa/dialogue_understanding/patterns/default_flows_for_patterns.yml index 917517f928ab..b836fd1498bd 100644 --- a/rasa/dialogue_understanding/patterns/default_flows_for_patterns.yml +++ b/rasa/dialogue_understanding/patterns/default_flows_for_patterns.yml @@ -41,7 +41,8 @@ slots: flows: pattern_continue_interrupted: - description: A flow that should will be started to continue an interrupted flow. + description: A flow that should will be started to continue an interrupted flow. + name: pattern continue interrupted steps: - id: "0" @@ -49,6 +50,7 @@ flows: pattern_correction: description: Handle a correction of a slot value. + name: pattern correction steps: - id: "check_if_reset_only" @@ -66,6 +68,7 @@ flows: pattern_cancel_flow: description: A meta flow that's started when a flow is cancelled. + name: pattern_cancel_flow steps: - id: "cancel_flow" @@ -76,18 +79,21 @@ flows: pattern_internal_error: description: internal error + name: pattern internal error steps: - id: "0" action: utter_internal_error_rasa pattern_completed: description: a flow has been completed and there is nothing else to be done + name: pattern completed steps: - id: "0" action: utter_can_do_something_else pattern_chitchat: description: handle interactions with the user that are not task-oriented + name: pattern chitchat steps: - id: "0" generation_prompt: | @@ -99,6 +105,7 @@ flows: pattern_clarification: description: handle clarifications with the user + name: pattern clarification steps: - id: "0" action: action_clarify_flows @@ -108,6 +115,7 @@ flows: pattern_collect_information: description: flow used to fill a slot + name: pattern collect information steps: - id: "start" action: action_run_slot_rejections diff --git a/rasa/validator.py b/rasa/validator.py index edc3b44ba8d2..d686362e13d1 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -1,4 +1,5 @@ import logging +import re import string from collections import defaultdict from typing import Set, Text, Optional, Dict, Any, List @@ -10,7 +11,6 @@ ActionFlowStep, BranchFlowStep, CollectInformationFlowStep, - Flow, FlowsList, IfFlowLink, SetSlotsFlowStep, @@ -528,10 +528,10 @@ def _raise_exception_if_dialogue_stack_slot( f"your flows.", ) - def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: + def verify_flows_steps_against_domain(self) -> bool: """Checks flows steps' references against the domain file.""" domain_slots = {slot.name: slot for slot in self.domain.slots} - for flow in user_flows: + for flow in self.flows.underlying_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): self._raise_exception_if_slot_not_in_domain( @@ -560,7 +560,17 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: ) elif isinstance(step, ActionFlowStep): - if step.action not in self.domain.action_names_or_texts: + regex = r"{context\..+?}" + matches = re.findall(regex, step.action) + if matches: + logger.warning( + f"An interpolated action name was found at step " + f"'{step.id}' of flow id '{flow.id}'. " + f"Skipping validation for this step. " + f"Please make sure that the action name is " + f"listed in your domain responses or actions." + ) + elif step.action not in self.domain.action_names_or_texts: raise RasaException( f"The action '{step.action}' is used in the step " f"'{step.id}' of flow id '{flow.id}', but it " @@ -569,13 +579,12 @@ def verify_flows_steps_against_domain(self, user_flows: List[Flow]) -> bool: ) return True - @staticmethod - def verify_unique_flows(user_flows: List[Flow]) -> bool: + def verify_unique_flows(self) -> bool: """Checks if all flows have unique names and descriptions.""" flows_mapping: Dict[str, str] = {} punctuation_table = str.maketrans({i: "" for i in string.punctuation}) - for flow in user_flows: + for flow in self.flows.underlying_flows: flow_description = flow.description cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 if cleaned_description in flows_mapping.values(): @@ -609,11 +618,10 @@ def _construct_predicate(predicate: Optional[str], step_id: str) -> Predicate: return pred - @staticmethod - def verify_predicates(user_flows: List[Flow]) -> bool: + def verify_predicates(self) -> bool: """Checks that predicates used in branch flow steps or `collect` steps are valid.""" # noqa: E501 all_good = True - for flow in user_flows: + for flow in self.flows.underlying_flows: for step in flow.steps: if isinstance(step, BranchFlowStep): for link in step.next.links: @@ -651,16 +659,10 @@ def verify_flows(self) -> bool: ) return True - user_flows = [ - flow - for flow in self.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] - all_good = ( - self.verify_flows_steps_against_domain(user_flows) - and self.verify_unique_flows(user_flows) - and self.verify_predicates(user_flows) + self.verify_flows_steps_against_domain() + and self.verify_unique_flows() + and self.verify_predicates() ) return all_good diff --git a/tests/test_validator.py b/tests/test_validator.py index 385ba238f3e7..1e324fba7842 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -1,3 +1,4 @@ +import logging import textwrap import warnings from typing import Any, Dict, List, Text @@ -935,14 +936,9 @@ def test_verify_flow_steps_against_domain_fail( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain(user_flows) + validator.verify_flows_steps_against_domain() assert exception_message in str(e.value) @@ -991,14 +987,9 @@ def test_verify_flow_steps_against_domain_disallowed_list_slot( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain(user_flows) + validator.verify_flows_steps_against_domain() assert ( "The slot 'pizza_toppings' is used in the step 'ask_pizza_toppings' " @@ -1041,14 +1032,9 @@ def test_verify_flow_steps_against_domain_dialogue_stack_slot( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain(user_flows) + validator.verify_flows_steps_against_domain() assert ( "The slot 'dialogue_stack' is used in the step 'ask_internal_slot' " @@ -1056,6 +1042,51 @@ def test_verify_flow_steps_against_domain_dialogue_stack_slot( ) +def test_verify_flow_steps_against_domain_interpolated_action_name( + caplog: LogCaptureFixture, + tmp_path: Path, + nlu_data_path: Path, +) -> None: + flows_file = tmp_path / "flows.yml" + with open(flows_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + flows: + pattern_collect_information: + description: Test that interpolated names log a warning. + name: test flow + steps: + - id: "validate" + action: "validate_{{context.collect}}" + """ + ) + domain_file = tmp_path / "domain.yml" + with open(domain_file, "w") as file: + file.write( + f""" + version: "{LATEST_TRAINING_DATA_FORMAT_VERSION}" + intents: + - greet + """ + ) + importer = RasaFileImporter( + config_file="data/test_moodbot/config.yml", + domain_path=str(domain_file), + training_data_paths=[str(flows_file), str(nlu_data_path)], + ) + + validator = Validator.from_importer(importer) + + with caplog.at_level(logging.WARNING): + assert validator.verify_flows_steps_against_domain() + assert ( + "An interpolated action name was found at step 'validate' " + "of flow id 'pattern_collect_information'. " + "Skipping validation for this step." in caplog.text + ) + + def test_verify_unique_flows_duplicate_names( tmp_path: Path, nlu_data_path: Path, @@ -1113,14 +1144,9 @@ def test_verify_unique_flows_duplicate_names( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_unique_flows(user_flows) + validator.verify_unique_flows() assert ( f"Detected duplicate flow name '{duplicate_flow_name}' for " @@ -1187,14 +1213,9 @@ def test_verify_unique_flows_duplicate_descriptions( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_unique_flows(user_flows) + validator.verify_unique_flows() assert ( "Detected duplicate flow description for flow id 'recurrent_payment'. " @@ -1272,13 +1293,8 @@ def test_verify_predicates_invalid_rejection_if( ) validator = Validator.from_importer(importer) - user_flows = [ - flow - for flow in validator.flows.underlying_flows - if not flow.id.startswith("pattern_") - ] with pytest.raises(RasaException) as e: - validator.verify_predicates(user_flows) + validator.verify_predicates() assert expected_exception in str(e.value) From e529e07c2d3307afedf95a9d10c14437e2b4e359 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Thu, 28 Sep 2023 15:54:49 +0100 Subject: [PATCH 09/10] convert exceptions to logging errors, adapt tests --- rasa/validator.py | 126 +++++++++++++++++++++++++--------------- tests/test_validator.py | 51 ++++++++-------- 2 files changed, 108 insertions(+), 69 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index d686362e13d1..0f2f43cb5d32 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -2,7 +2,7 @@ import re import string from collections import defaultdict -from typing import Set, Text, Optional, Dict, Any, List +from typing import Set, Text, Optional, Dict, Any, List, Tuple from pypred import Predicate @@ -36,7 +36,6 @@ from rasa.shared.core.constants import SlotMappingType, MAPPING_TYPE from rasa.shared.core.slots import ListSlot, Slot from rasa.shared.core.training_data.structures import StoryGraph -from rasa.shared.exceptions import RasaException from rasa.shared.importers.importer import TrainingDataImporter from rasa.shared.nlu.training_data.training_data import TrainingData import rasa.shared.utils.io @@ -494,69 +493,91 @@ def warn_if_config_mandatory_keys_are_not_set(self) -> None: ) @staticmethod - def _raise_exception_if_slot_not_in_domain( - slot_name: str, domain_slots: Dict[Text, Slot], step_id: str, flow_id: str - ) -> None: + def _log_error_if_slot_not_in_domain( + slot_name: str, + domain_slots: Dict[Text, Slot], + step_id: str, + flow_id: str, + all_good: bool, + ) -> bool: if slot_name not in domain_slots: - raise RasaException( + logger.error( f"The slot '{slot_name}' is used in the " f"step '{step_id}' of flow id '{flow_id}', but it " f"is not listed in the domain slots. " f"You should add it to your domain file!", ) + all_good = False + + return all_good @staticmethod - def _raise_exception_if_list_slot(slot: Slot, step_id: str, flow_id: str) -> None: + def _log_error_if_list_slot( + slot: Slot, step_id: str, flow_id: str, all_good: bool + ) -> bool: if isinstance(slot, ListSlot): - raise RasaException( + logger.error( f"The slot '{slot.name}' is used in the " f"step '{step_id}' of flow id '{flow_id}', but it " f"is a list slot. List slots are currently not " f"supported in flows. You should change it to a " f"text, boolean or float slot in your domain file!", ) + all_good = False + + return all_good @staticmethod - def _raise_exception_if_dialogue_stack_slot( - slot: Slot, step_id: str, flow_id: str - ) -> None: + def _log_error_if_dialogue_stack_slot( + slot: Slot, step_id: str, flow_id: str, all_good: bool + ) -> bool: if slot.name == constants.DIALOGUE_STACK_SLOT: - raise RasaException( + logger.error( f"The slot '{constants.DIALOGUE_STACK_SLOT}' is used in the " f"step '{step_id}' of flow id '{flow_id}', but it " f"is a reserved slot. You must not use reserved slots in " f"your flows.", ) + all_good = False + + return all_good def verify_flows_steps_against_domain(self) -> bool: """Checks flows steps' references against the domain file.""" + all_good = True domain_slots = {slot.name: slot for slot in self.domain.slots} for flow in self.flows.underlying_flows: for step in flow.steps: if isinstance(step, CollectInformationFlowStep): - self._raise_exception_if_slot_not_in_domain( - step.collect, domain_slots, step.id, flow.id + all_good = self._log_error_if_slot_not_in_domain( + step.collect, domain_slots, step.id, flow.id, all_good ) + if not all_good: + continue current_slot = domain_slots[step.collect] - self._raise_exception_if_list_slot(current_slot, step.id, flow.id) - self._raise_exception_if_dialogue_stack_slot( - current_slot, step.id, flow.id + all_good = self._log_error_if_list_slot( + current_slot, step.id, flow.id, all_good + ) + all_good = self._log_error_if_dialogue_stack_slot( + current_slot, step.id, flow.id, all_good ) elif isinstance(step, SetSlotsFlowStep): for slot in step.slots: slot_name = slot["key"] - self._raise_exception_if_slot_not_in_domain( - slot_name, domain_slots, step.id, flow.id + all_good = self._log_error_if_slot_not_in_domain( + slot_name, domain_slots, step.id, flow.id, all_good ) + if not all_good: + continue current_slot = domain_slots[slot_name] - self._raise_exception_if_list_slot( - current_slot, step.id, flow.id + all_good = self._log_error_if_list_slot( + current_slot, step.id, flow.id, all_good ) - self._raise_exception_if_dialogue_stack_slot( - current_slot, step.id, flow.id + all_good = self._log_error_if_dialogue_stack_slot( + current_slot, step.id, flow.id, all_good ) elif isinstance(step, ActionFlowStep): @@ -564,59 +585,68 @@ def verify_flows_steps_against_domain(self) -> bool: matches = re.findall(regex, step.action) if matches: logger.warning( - f"An interpolated action name was found at step " - f"'{step.id}' of flow id '{flow.id}'. " + f"An interpolated action name '{step.action}' was " + f"found at step '{step.id}' of flow id '{flow.id}'. " f"Skipping validation for this step. " f"Please make sure that the action name is " f"listed in your domain responses or actions." ) elif step.action not in self.domain.action_names_or_texts: - raise RasaException( + logger.error( f"The action '{step.action}' is used in the step " f"'{step.id}' of flow id '{flow.id}', but it " f"is not listed in the domain file. " f"You should add it to your domain file!", ) - return True + all_good = False + return all_good def verify_unique_flows(self) -> bool: """Checks if all flows have unique names and descriptions.""" - flows_mapping: Dict[str, str] = {} + all_good = True + flow_names = set() + flow_descriptions = set() punctuation_table = str.maketrans({i: "" for i in string.punctuation}) for flow in self.flows.underlying_flows: flow_description = flow.description cleaned_description = flow_description.translate(punctuation_table) # type: ignore[union-attr] # noqa: E501 - if cleaned_description in flows_mapping.values(): - raise RasaException( + if cleaned_description in flow_descriptions: + logger.error( f"Detected duplicate flow description for flow id '{flow.id}'. " f"Flow descriptions must be unique. " f"Please make sure that all flows have different descriptions." ) + all_good = False - if flow.name in flows_mapping: - raise RasaException( + if flow.name in flow_names: + logger.error( f"Detected duplicate flow name '{flow.name}' for flow " f"id '{flow.id}'. Flow names must be unique. " f"Please make sure that all flows have different names." ) + all_good = False - flows_mapping[flow.name] = cleaned_description + flow_names.add(flow.name) + flow_descriptions.add(cleaned_description) - return True + return all_good @staticmethod - def _construct_predicate(predicate: Optional[str], step_id: str) -> Predicate: + def _construct_predicate( + predicate: Optional[str], step_id: str, all_good: bool = True + ) -> Tuple[Optional[Predicate], bool]: try: pred = Predicate(predicate) except (TypeError, Exception) as exception: - raise RasaException( + logger.error( f"Could not initialize the predicate found under step " - f"'{step_id}'. Please make sure that all predicates " - f"are strings." - ) from exception + f"'{step_id}': {exception}." + ) + pred = None + all_good = False - return pred + return pred, all_good def verify_predicates(self) -> bool: """Checks that predicates used in branch flow steps or `collect` steps are valid.""" # noqa: E501 @@ -626,26 +656,30 @@ def verify_predicates(self) -> bool: if isinstance(step, BranchFlowStep): for link in step.next.links: if isinstance(link, IfFlowLink): - predicate = Validator._construct_predicate( + predicate, all_good = Validator._construct_predicate( link.condition, step.id ) - if not predicate.is_valid(): - raise RasaException( + if predicate and not predicate.is_valid(): + logger.error( f"Detected invalid condition '{link.condition}' " f"at step '{step.id}' for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) + all_good = False elif isinstance(step, CollectInformationFlowStep): predicates = [predicate.if_ for predicate in step.rejections] for predicate in predicates: - pred = Validator._construct_predicate(predicate, step.id) - if not pred.is_valid(): - raise RasaException( + pred, all_good = Validator._construct_predicate( + predicate, step.id + ) + if pred and not pred.is_valid(): + logger.error( f"Detected invalid rejection '{predicate}' " f"at `collect` step '{step.id}' " f"for flow id '{flow.id}'. " f"Please make sure that all conditions are valid." ) + all_good = False return all_good def verify_flows(self) -> bool: diff --git a/tests/test_validator.py b/tests/test_validator.py index 1e324fba7842..d52ecd4b9d9b 100644 --- a/tests/test_validator.py +++ b/tests/test_validator.py @@ -6,7 +6,6 @@ import pytest from _pytest.logging import LogCaptureFixture from rasa.shared.constants import LATEST_TRAINING_DATA_FORMAT_VERSION -from rasa.shared.exceptions import RasaException from rasa.validator import Validator @@ -861,7 +860,7 @@ def test_warn_if_config_mandatory_keys_are_not_set_invalid_paths( @pytest.mark.parametrize( - "domain_actions, domain_slots, exception_message", + "domain_actions, domain_slots, log_message", [ # set_slot slot is not listed in the domain ( @@ -894,7 +893,8 @@ def test_verify_flow_steps_against_domain_fail( nlu_data_path: Path, domain_actions: List[Text], domain_slots: Dict[Text, Any], - exception_message: Text, + log_message: Text, + caplog: LogCaptureFixture, ) -> None: flows_file = tmp_path / "flows.yml" with open(flows_file, "w") as file: @@ -937,15 +937,16 @@ def test_verify_flow_steps_against_domain_fail( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + with caplog.at_level(logging.ERROR): + assert not validator.verify_flows_steps_against_domain() - assert exception_message in str(e.value) + assert log_message in caplog.text def test_verify_flow_steps_against_domain_disallowed_list_slot( tmp_path: Path, nlu_data_path: Path, + caplog: LogCaptureFixture, ) -> None: flows_file = tmp_path / "flows.yml" with open(flows_file, "w") as file: @@ -988,19 +989,20 @@ def test_verify_flow_steps_against_domain_disallowed_list_slot( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + with caplog.at_level(logging.ERROR): + assert not validator.verify_flows_steps_against_domain() assert ( "The slot 'pizza_toppings' is used in the step 'ask_pizza_toppings' " "of flow id 'order_pizza', but it is a list slot. List slots are " - "currently not supported in flows." in str(e.value) + "currently not supported in flows." in caplog.text ) def test_verify_flow_steps_against_domain_dialogue_stack_slot( tmp_path: Path, nlu_data_path: Path, + caplog: LogCaptureFixture, ) -> None: flows_file = tmp_path / "flows.yml" with open(flows_file, "w") as file: @@ -1033,12 +1035,12 @@ def test_verify_flow_steps_against_domain_dialogue_stack_slot( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: - validator.verify_flows_steps_against_domain() + with caplog.at_level(logging.ERROR): + assert not validator.verify_flows_steps_against_domain() assert ( "The slot 'dialogue_stack' is used in the step 'ask_internal_slot' " - "of flow id 'my_flow', but it is a reserved slot." in str(e.value) + "of flow id 'my_flow', but it is a reserved slot." in caplog.text ) @@ -1081,8 +1083,8 @@ def test_verify_flow_steps_against_domain_interpolated_action_name( with caplog.at_level(logging.WARNING): assert validator.verify_flows_steps_against_domain() assert ( - "An interpolated action name was found at step 'validate' " - "of flow id 'pattern_collect_information'. " + "An interpolated action name 'validate_{context.collect}' was found " + "at step 'validate' of flow id 'pattern_collect_information'. " "Skipping validation for this step." in caplog.text ) @@ -1090,6 +1092,7 @@ def test_verify_flow_steps_against_domain_interpolated_action_name( def test_verify_unique_flows_duplicate_names( tmp_path: Path, nlu_data_path: Path, + caplog: LogCaptureFixture, ) -> None: duplicate_flow_name = "transfer money" flows_file = tmp_path / "flows.yml" @@ -1145,19 +1148,20 @@ def test_verify_unique_flows_duplicate_names( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: - validator.verify_unique_flows() + with caplog.at_level(logging.ERROR): + assert not validator.verify_unique_flows() assert ( f"Detected duplicate flow name '{duplicate_flow_name}' for " f"flow id 'recurrent_payment'. Flow names must be unique. " f"Please make sure that all flows have different names." - ) in str(e.value) + ) in caplog.text def test_verify_unique_flows_duplicate_descriptions( tmp_path: Path, nlu_data_path: Path, + caplog: LogCaptureFixture, ) -> None: duplicate_flow_description_with_punctuation = "This flow lets users send money." duplicate_flow_description = "This flow lets users send money" @@ -1214,22 +1218,23 @@ def test_verify_unique_flows_duplicate_descriptions( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: + with caplog.at_level(logging.ERROR): validator.verify_unique_flows() assert ( "Detected duplicate flow description for flow id 'recurrent_payment'. " "Flow descriptions must be unique. " "Please make sure that all flows have different descriptions." - ) in str(e.value) + ) in caplog.text def test_verify_predicates_invalid_rejection_if( tmp_path: Path, nlu_data_path: Path, + caplog: LogCaptureFixture, ) -> None: predicate = 'account_type not in {{"debit", "savings"}}' - expected_exception = ( + error_log = ( f"Detected invalid rejection '{predicate}' " f"at `collect` step 'ask_account_type' " f"for flow id 'transfer_money'. " @@ -1294,7 +1299,7 @@ def test_verify_predicates_invalid_rejection_if( validator = Validator.from_importer(importer) - with pytest.raises(RasaException) as e: - validator.verify_predicates() + with caplog.at_level(logging.ERROR): + assert not validator.verify_predicates() - assert expected_exception in str(e.value) + assert error_log in caplog.text From d465324c717d33cef55913c743f236ea7cfed235 Mon Sep 17 00:00:00 2001 From: Anca Lita <27920906+ancalita@users.noreply.github.com> Date: Fri, 29 Sep 2023 07:58:28 +0100 Subject: [PATCH 10/10] add some fixes --- rasa/validator.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/rasa/validator.py b/rasa/validator.py index 0f2f43cb5d32..2a157d5105b5 100644 --- a/rasa/validator.py +++ b/rasa/validator.py @@ -552,10 +552,10 @@ def verify_flows_steps_against_domain(self) -> bool: all_good = self._log_error_if_slot_not_in_domain( step.collect, domain_slots, step.id, flow.id, all_good ) - if not all_good: + current_slot = domain_slots.get(step.collect) + if not current_slot: continue - current_slot = domain_slots[step.collect] all_good = self._log_error_if_list_slot( current_slot, step.id, flow.id, all_good ) @@ -569,10 +569,10 @@ def verify_flows_steps_against_domain(self) -> bool: all_good = self._log_error_if_slot_not_in_domain( slot_name, domain_slots, step.id, flow.id, all_good ) - if not all_good: + current_slot = domain_slots.get(slot_name) + if not current_slot: continue - current_slot = domain_slots[slot_name] all_good = self._log_error_if_list_slot( current_slot, step.id, flow.id, all_good ) @@ -693,10 +693,10 @@ def verify_flows(self) -> bool: ) return True - all_good = ( - self.verify_flows_steps_against_domain() - and self.verify_unique_flows() - and self.verify_predicates() - ) + condition_one = self.verify_flows_steps_against_domain() + condition_two = self.verify_unique_flows() + condition_three = self.verify_predicates() + + all_good = all([condition_one, condition_two, condition_three]) return all_good