Merge branch 'main' into 10144/ValidationAction-fail-error

changelog/10079.bugfix.md

@@ -0,0 +1 @@
+Fixed validation behavior and logging output around unused intents and utterances.

data/test_config/config_defaults.yml

@@ -24,3 +24,16 @@ pipeline: []
 #     ambiguity_threshold: 0.1
 
 data:
+policies:
+# # No configuration for policies was provided. The following default policies were used to train your model.
+# # If you'd like to customize them, uncomment and adjust the policies.
+# # See https://rasa.com/docs/rasa/policies for more information.
+#   - name: MemoizationPolicy
+#   - name: RulePolicy
+#   - name: UnexpecTEDIntentPolicy
+#     max_history: 5
+#     epochs: 100
+#   - name: TEDPolicy
+#     max_history: 5
+#     epochs: 100
+#     constrain_similarities: true

data/test_validation/data/nlu.yml

@@ -0,0 +1,5 @@
+version: "2.0"
+nlu:
+- intent: greet
+  examples: |
+    - hey

data/test_validation/data/stories.yml

@@ -0,0 +1,6 @@
+version: "2.0"
+stories:
+- story: happy path
+  steps:
+  - intent: greet
+  - action: utter_greet

data/test_validation/domain.yml

@@ -0,0 +1,12 @@
+version: "2.0"
+
+intents:
+  - greet
+  - goodbye
+
+responses:
+  utter_greet:
+    - text: "Hey! How are you?"
+
+  utter_chatter:
+    - text: "Lovely weather today, isn't it?"

rasa/core/migrate.py

@@ -1,4 +1,5 @@
 import copy
+import shutil
 from pathlib import Path
 from typing import List, Dict, Text, Any, Tuple, Optional, Union
 
@@ -180,75 +181,90 @@ def _write_final_domain(
         rasa.shared.utils.io.write_yaml(new_domain, out_file, True)
 
 
+def _migrate_domain_files(
+    domain_file: Path, backup_dir: Path, out_file: Path
+) -> Dict[Text, Any]:
+    slots = {}
+    forms = {}
+    entities = []
+
+    for file in domain_file.iterdir():
+        if not Domain.is_domain_file(file):
+            continue
+
+        backup = backup_dir / file.name
+        original_content = _create_back_up(file, backup)
+
+        if KEY_SLOTS not in original_content and KEY_FORMS not in original_content:
+            # this is done so that the other domain files can be moved
+            # in the migrated directory
+            rasa.shared.utils.io.write_yaml(
+                original_content, out_file / file.name, True
+            )
+        elif KEY_SLOTS in original_content and slots:
+            raise RasaException(
+                f"Domain files with multiple '{KEY_SLOTS}' "
+                f"sections were provided. Please group these sections "
+                f"in one file only to prevent content duplication across "
+                f"multiple files. "
+            )
+        elif KEY_FORMS in original_content and forms:
+            raise RasaException(
+                f"Domain files with multiple '{KEY_FORMS}' "
+                f"sections were provided. Please group these sections "
+                f"in one file only to prevent content duplication across "
+                f"multiple files. "
+            )
+
+        slots.update(original_content.get(KEY_SLOTS, {}))
+        forms.update(original_content.get(KEY_FORMS, {}))
+        entities.extend(original_content.get(KEY_ENTITIES, {}))
+
+    if not slots or not forms:
+        raise RasaException(
+            f"The files you have provided in '{domain_file}' are missing slots "
+            f"or forms. Please make sure to include these for a "
+            f"successful migration."
+        )
+
+    return {KEY_SLOTS: slots, KEY_FORMS: forms, KEY_ENTITIES: entities}
+
+
 def migrate_domain_format(
     domain_file: Union[Text, Path], out_file: Union[Text, Path]
 ) -> None:
     """Converts 2.0 domain to 3.0 format."""
     domain_file = Path(domain_file)
     out_file = Path(out_file)
+    created_out_dir = False
 
     current_dir = domain_file.parent
 
     if domain_file.is_dir():
         backup_dir = current_dir / "original_domain"
         backup_dir.mkdir()
 
-        slots = {}
-        forms = {}
-        entities = []
-        original_domain = {}
-
         if out_file.is_file() or not out_file.exists():
             out_file = current_dir / "new_domain"
             out_file.mkdir()
+            created_out_dir = True
             rasa.shared.utils.io.raise_warning(
                 f"The out path provided is not a directory, "
                 f"creating a new directory '{str(out_file)}' "
                 f"for migrated domain files."
             )
 
-        for file in domain_file.iterdir():
-            if not Domain.is_domain_file(file):
-                continue
-
-            backup = backup_dir / file.name
-            original_content = _create_back_up(file, backup)
-
-            if KEY_SLOTS not in original_content and KEY_FORMS not in original_content:
-                # this is done so that the other domain files can be moved
-                # in the migrated directory
-                rasa.shared.utils.io.write_yaml(
-                    original_content, out_file / file.name, True
-                )
-            elif KEY_SLOTS in original_content and slots:
-                raise RasaException(
-                    f"Domain files with multiple '{KEY_SLOTS}' "
-                    f"sections were provided. Please group these sections "
-                    f"in one file only to prevent content duplication across "
-                    f"multiple files. "
-                )
-            elif KEY_FORMS in original_content and forms:
-                raise RasaException(
-                    f"Domain files with multiple '{KEY_FORMS}' "
-                    f"sections were provided. Please group these sections "
-                    f"in one file only to prevent content duplication across "
-                    f"multiple files. "
-                )
-
-            slots.update(original_content.get(KEY_SLOTS, {}))
-            forms.update(original_content.get(KEY_FORMS, {}))
-            entities.extend(original_content.get(KEY_ENTITIES, {}))
-
-        if not slots or not forms:
-            raise RasaException(
-                f"The files you have provided in '{domain_file}' are missing slots "
-                f"or forms. Please make sure to include these for a "
-                f"successful migration."
-            )
-
-        original_domain.update(
-            {KEY_SLOTS: slots, KEY_FORMS: forms, KEY_ENTITIES: entities}
-        )
+        try:
+            original_domain = _migrate_domain_files(domain_file, backup_dir, out_file)
+        except Exception as e:
+            shutil.rmtree(backup_dir)
+            if out_file != domain_file:
+                shutil.rmtree(out_file)
+                if not created_out_dir:
+                    # we recreate the deleted directory
+                    # because it existed before
+                    out_file.mkdir()
+            raise e
     else:
         if not Domain.is_domain_file(domain_file):
             raise RasaException(

rasa/validator.py

@@ -1,7 +1,7 @@
 import itertools
 import logging
 from collections import defaultdict
-from typing import Set, Text, Optional, Dict, Any
+from typing import Set, Text, Optional, Dict, Any, List
 
 import rasa.core.training.story_conflict
 import rasa.shared.nlu.constants
@@ -12,6 +12,7 @@
     DOCS_URL_ACTIONS,
     REQUIRED_SLOTS_KEY,
 )
+from rasa.shared.core import constants
 from rasa.shared.core.constants import MAPPING_CONDITIONS, ACTIVE_LOOP
 from rasa.shared.core.domain import Domain
 from rasa.shared.core.events import ActionExecuted, ActiveLoop
@@ -58,15 +59,22 @@ def from_importer(cls, importer: TrainingDataImporter) -> "Validator":
 
         return cls(domain, intents, story_graph, config)
 
+    def _non_default_intents(self) -> List[Text]:
+        return [
+            item
+            for item in self.domain.intents
+            if item not in constants.DEFAULT_INTENTS
+        ]
+
     def verify_intents(self, ignore_warnings: bool = True) -> bool:
         """Compares list of intents in domain with intents in NLU training data."""
         everything_is_alright = True
 
         nlu_data_intents = {e.data["intent"] for e in self.intents.intent_examples}
 
-        for intent in self.domain.intents:
+        for intent in self._non_default_intents():
             if intent not in nlu_data_intents:
-                logger.debug(
+                rasa.shared.utils.io.raise_warning(
                     f"The intent '{intent}' is listed in the domain file, but "
                     f"is not found in the NLU training data."
                 )
@@ -134,9 +142,11 @@ def verify_intents_in_stories(self, ignore_warnings: bool = True) -> bool:
                 )
                 everything_is_alright = False
 
-        for intent in self.domain.intents:
+        for intent in self._non_default_intents():
             if intent not in stories_intents:
-                logger.debug(f"The intent '{intent}' is not used in any story.")
+                rasa.shared.utils.io.raise_warning(
+                    f"The intent '{intent}' is not used in any story or rule."
+                )
                 everything_is_alright = ignore_warnings and everything_is_alright
 
         return everything_is_alright
@@ -195,7 +205,9 @@ def verify_utterances_in_stories(self, ignore_warnings: bool = True) -> bool:
 
         for utterance in utterance_actions:
             if utterance not in stories_utterances:
-                logger.debug(f"The utterance '{utterance}' is not used in any story.")
+                rasa.shared.utils.io.raise_warning(
+                    f"The utterance '{utterance}' is not used in any story or rule."
+                )
                 everything_is_alright = ignore_warnings and everything_is_alright
 
         return everything_is_alright

tests/core/test_migrate.py

@@ -1,3 +1,4 @@
+import os
 import re
 import textwrap
 from pathlib import Path
@@ -764,3 +765,88 @@ def test_migrate_domain_raises_for_non_domain_files(tmp_path: Path):
         f"Please make sure to include these for a successful migration.",
     ):
         rasa.core.migrate.migrate_domain_format(domain_dir, domain_dir)
+
+
+def test_migration_cleanup(tmp_path: Path,):
+    domain_dir = tmp_path / "domain"
+    domain_dir.mkdir()
+    migrated_domain_dir = tmp_path / "domain2"
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          outdoor_seating:
+           type: bool
+           influence_conversation: false
+        """,
+        "slots_one.yml",
+    )
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          cuisine:
+           type: text
+           influence_conversation: false
+        """,
+        "slots_two.yml",
+    )
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        responses:
+          utter_greet:
+          - text: "Hi there!"
+        """,
+        "responses.yml",
+    )
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, domain_dir)
+
+    # here the migration fails but we check if the domain was
+    # not deleted in the process of cleanup
+    assert Path.exists(domain_dir)
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    # the migration fails again and we check
+    # if the cleanup was done successfully
+    current_dir = domain_dir.parent
+    assert Path.exists(domain_dir)
+    assert not Path.exists(current_dir / "original_domain")
+    assert not Path.exists(current_dir / "new_domain")
+
+    # create the `migrated_domain_dir` to use it instead of the default one
+    migrated_domain_dir.mkdir()
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    # check if the cleanup was done successfully after
+    # the `migrated_domain_dir` was created
+    assert (
+        Path.exists(migrated_domain_dir) and len(os.listdir(migrated_domain_dir)) == 0
+    )
+    assert not Path.exists(current_dir / "original_domain")
+    assert not Path.exists(current_dir / "new_domain")