Repository: RasaHQ/rasa
Pull request: support loading of files from additional training paths
**Proposed changes**:
- fixes https://github.com/RasaHQ/rasa/issues/3633

**Status (please check what you already did)**:
- [x] made PR ready for code review
- [x] added some tests for the functionality
- [ ] updated the documentation
- [ ] updated the changelog
- [x] reformat files using `black` (please check [Readme](https://github.com/RasaHQ/rasa_nlu#code-style) for instructions)


Problem statement:
Multi Domain loading not importing data directory of root project
Currently the experimental multi bot feature does only load data specified in imported bots or in the root project directory. It should also load data in the data directory of the root project (project in which the command is executed).

Hints from issue discussion:
Great work Tobias. What would happen in case of name conflicts between skills? Should skills be namespaced eg. action would be called `GreetBot.bye`, same with slots.
At the moment, everything is merged. But we plan to add some support to scope the imports.

Commit message: update changelog

Patch to review:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 33e1cfd806d9..0a5ac3cd30d2 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -23,6 +23,7 @@ Fixed
 - slack notifications from bots correctly render text
 - fixed usage of ``--log-file`` argument for ``rasa run`` and ``rasa shell``
 - check if correct tracker store is configured in local mode
+- loading of additional training data loading with the ``SkillSelector``
 
 [1.0.6] - 2019-06-03
 ^^^^^^^^^^^^^^^^^^^^
diff --git a/data/test_multi_domain/data/GreetBot/data/nlu.md b/data/test_multi_domain/data/GreetBot/data/nlu.md
index c3af526c9e17..d697b0c6c61d 100644
--- a/data/test_multi_domain/data/GreetBot/data/nlu.md
+++ b/data/test_multi_domain/data/GreetBot/data/nlu.md
@@ -13,15 +13,3 @@
 - goodevening
 - good afternoon
 
-## intent:goodbye
-- cu
-- good by
-- cee you later
-- good night
-- good afternoon
-- bye
-- goodbye
-- have a nice day
-- see you around
-- bye bye
-- see you later
diff --git a/data/test_multi_domain/data/GreetBot/data/stories.md b/data/test_multi_domain/data/GreetBot/data/stories.md
index 3e1bfa50a6b7..ba800bcf75ef 100644
--- a/data/test_multi_domain/data/GreetBot/data/stories.md
+++ b/data/test_multi_domain/data/GreetBot/data/stories.md
@@ -1,7 +1,3 @@
-## happy path
+## say hello
 * greet
   - utter_greet
-
-## say goodbye
-* goodbye
-  - utter_goodbye
diff --git a/data/test_multi_domain/data/nlu.md b/data/test_multi_domain/data/nlu.md
new file mode 100644
index 000000000000..56c625bcbb17
--- /dev/null
+++ b/data/test_multi_domain/data/nlu.md
@@ -0,0 +1,12 @@
+## intent:goodbye
+- cu
+- good by
+- cee you later
+- good night
+- good afternoon
+- bye
+- goodbye
+- have a nice day
+- see you around
+- bye bye
+- see you later
diff --git a/data/test_multi_domain/data/stories.md b/data/test_multi_domain/data/stories.md
new file mode 100644
index 000000000000..04cb9c36d77c
--- /dev/null
+++ b/data/test_multi_domain/data/stories.md
@@ -0,0 +1,3 @@
+## say goodbye
+* goodbye
+  - utter_goodbye
diff --git a/data/test_multi_domain/domain.yml b/data/test_multi_domain/domain.yml
new file mode 100644
index 000000000000..f0915841e2ee
--- /dev/null
+++ b/data/test_multi_domain/domain.yml
@@ -0,0 +1,9 @@
+intents:
+  - goodbye
+
+actions:
+- utter_goodbye
+
+templates:
+  utter_goodbye:
+    - text: "Bye"
diff --git a/rasa/skill.py b/rasa/skill.py
index ecde7e1483f2..e47f8eed9364 100644
--- a/rasa/skill.py
+++ b/rasa/skill.py
@@ -1,5 +1,5 @@
 import logging
-from typing import Text, Set, Dict
+from typing import Text, Set, Dict, Optional, List, Union
 import os
 
 import rasa.utils.io as io_utils
@@ -9,22 +9,47 @@
 
 
 class SkillSelector:
-    def __init__(self, imports: Set[Text], project_directory: Text = os.getcwd()):
+    def __init__(
+        self,
+        imports: Set[Text],
+        project_directory: Text = os.getcwd(),
+        additional_paths: Optional[Union[Text, List[Text]]] = None,
+    ):
         self._imports = imports
         self._project_directory = project_directory
+        self._additional_paths = self._get_additional_training_paths(additional_paths)
+
+    @staticmethod
+    def _get_additional_training_paths(
+        paths: Optional[Union[Text, List[Text]]]
+    ) -> List[Text]:
+        additional_training_paths = paths or []
+        if not isinstance(additional_training_paths, list):
+            additional_training_paths = [additional_training_paths]
+
+        return [os.path.abspath(p) for p in additional_training_paths]
 
     @classmethod
-    def all_skills(cls, project_directory: Text = os.getcwd()) -> "SkillSelector":
+    def all_skills(
+        cls,
+        project_directory: Text = os.getcwd(),
+        project_data_paths: Optional[Union[Text, List[Text]]] = None,
+    ) -> "SkillSelector":
         """Returns a `SkillSelector` instance which does not specify any skills."""
 
-        return cls(set(), project_directory)
+        return cls(set(), project_directory, project_data_paths)
 
     @classmethod
-    def load(cls, config: Text) -> "SkillSelector":
+    def load(
+        cls,
+        config: Text,
+        additional_training_paths: Optional[Union[Text, List[Text]]] = None,
+    ) -> "SkillSelector":
         """
         Loads the specification from the config files.
         Args:
             config: Path to the root configuration file in the project directory.
+            additional_training_paths: Paths to additional training files.
 
         Returns:
             `SkillSelector` which specifies the loaded skills.
@@ -34,13 +59,13 @@ def load(cls, config: Text) -> "SkillSelector":
 
         # Create a base selector which keeps track of the imports during the
         # skill config loading in order to avoid cyclic imports
-        selector = cls.all_skills(os.path.dirname(config))
+        selector = cls.all_skills(os.path.dirname(config), additional_training_paths)
 
         selector = cls._from_file(config, selector)
 
         logger.debug(
-            "Selected skills: {}.".format(
-                ["\n- {}".format(i) for i in selector._imports]
+            "Selected skills: {}".format(
+                "".join(["\n-{}".format(i) for i in selector._imports])
             )
         )
 
@@ -116,7 +141,7 @@ def merge(self, other: "SkillSelector") -> "SkillSelector":
             }
         )
 
-        return SkillSelector(imports, self._project_directory)
+        return SkillSelector(imports, self._project_directory, self._additional_paths)
 
     def no_skills_selected(self) -> bool:
         return not self._imports
@@ -150,6 +175,7 @@ def is_imported(self, path: Text) -> bool:
         return (
             self.no_skills_selected()
             or self._is_in_project_directory(absolute_path)
+            or self._is_in_additional_paths(absolute_path)
             or self._is_in_imported_paths(absolute_path)
         )
 
@@ -161,6 +187,15 @@ def _is_in_project_directory(self, path: Text) -> bool:
         else:
             return path == self._project_directory
 
+    def _is_in_additional_paths(self, path: Text) -> bool:
+        included = path in self._additional_paths
+
+        if not included and os.path.isfile(path):
+            parent_directory = os.path.abspath(os.path.dirname(path))
+            included = parent_directory in self._additional_paths
+
+        return included
+
     def _is_in_imported_paths(self, path):
         return any([io_utils.is_subdirectory(path, i) for i in self._imports])
 
diff --git a/rasa/train.py b/rasa/train.py
index 05aa6afc7ddb..dad3672583db 100644
--- a/rasa/train.py
+++ b/rasa/train.py
@@ -76,7 +76,7 @@ async def train_async(
     config = _get_valid_config(config, CONFIG_MANDATORY_KEYS)
     train_path = tempfile.mkdtemp()
 
-    skill_imports = SkillSelector.load(config)
+    skill_imports = SkillSelector.load(config, training_files)
     try:
         domain = Domain.load(domain, skill_imports)
     except InvalidDomain as e:
@@ -254,7 +254,7 @@ async def train_core_async(
     """
 
     config = _get_valid_config(config, CONFIG_MANDATORY_KEYS_CORE)
-    skill_imports = SkillSelector.load(config)
+    skill_imports = SkillSelector.load(config, stories)
 
     if isinstance(domain, str):
         try:
@@ -354,7 +354,7 @@ def train_nlu(
     config = _get_valid_config(config, CONFIG_MANDATORY_KEYS_NLU)
 
     # training NLU only hence the training files still have to be selected
-    skill_imports = SkillSelector.load(config)
+    skill_imports = SkillSelector.load(config, nlu_data)
     nlu_data_directory = data.get_nlu_directory(nlu_data, skill_imports)
 
     if not os.listdir(nlu_data_directory):
diff --git a/tests/test_skill.py b/tests/test_skill.py
index c8d9cabb916e..941ecdf09077 100644
--- a/tests/test_skill.py
+++ b/tests/test_skill.py
@@ -4,6 +4,7 @@
 from _pytest.tmpdir import TempdirFactory
 import os
 
+from rasa.nlu.training_data.formats import RasaReader
 from rasa import model
 from rasa.core import utils
 from rasa.core.domain import Domain
@@ -156,33 +157,95 @@ def test_import_outside_project_directory(tmpdir_factory):
     assert actual._imports == {str(skill_b_directory), str(root / "Skill C")}
 
 
+def test_importing_additional_files(tmpdir_factory):
+    root = tmpdir_factory.mktemp("Parent Bot")
+    config = {"imports": ["bots/Bot A"]}
+    config_path = str(root / "config.yml")
+    utils.dump_obj_as_yaml_to_file(config_path, config)
+
+    additional_file = root / "directory" / "file.yml"
+    selector = SkillSelector.load(
+        config_path, [str(root / "data"), str(additional_file)]
+    )
+    # create intermediate directories and fake files
+    additional_file.write({}, ensure=True)
+    additional_directory = root / "data"
+    (additional_directory / "file.yml").write({}, ensure=True)
+
+    assert selector.is_imported(str(additional_directory))
+    assert selector.is_imported(str(additional_directory / "file.yml"))
+
+    assert selector.is_imported(str(additional_file))
+
+
+def test_not_importing_not_relevant_additional_files(tmpdir_factory):
+    root = tmpdir_factory.mktemp("Parent Bot")
+    config = {"imports": ["bots/Bot A"]}
+    config_path = str(root / "config.yml")
+    utils.dump_obj_as_yaml_to_file(config_path, config)
+
+    additional_file = root / "directory" / "file.yml"
+    selector = SkillSelector.load(
+        config_path, [str(root / "data"), str(additional_file)]
+    )
+
+    not_relevant_file1 = root / "data" / "another directory" / "file.yml"
+    not_relevant_file1.write({}, ensure=True)
+    not_relevant_file2 = root / "directory" / "another_file.yml"
+    not_relevant_file2.write({}, ensure=True)
+
+    assert not selector.is_imported(str(not_relevant_file1))
+    assert not selector.is_imported(str(not_relevant_file2))
+
+
+def test_single_additional_file(tmpdir_factory):
+    root = tmpdir_factory.mktemp("Parent Bot")
+    config_path = str(root / "config.yml")
+    empty_config = {}
+    utils.dump_obj_as_yaml_to_file(config_path, empty_config)
+
+    additional_file = root / "directory" / "file.yml"
+    additional_file.write({}, ensure=True)
+
+    selector = SkillSelector.load(config_path, str(additional_file))
+    assert isinstance(selector._additional_paths, list)
+
+    assert selector.is_imported(str(additional_file))
+
+
 async def test_multi_skill_training():
     example_directory = "data/test_multi_domain"
     config_file = os.path.join(example_directory, "config.yml")
+    files_of_root_project = os.path.join(example_directory, "data")
     trained_stack_model_path = await train_async(
-        config=config_file, domain=None, training_files=None
+        config=config_file, domain=None, training_files=files_of_root_project
     )
 
     unpacked = model.unpack_model(trained_stack_model_path)
     model_fingerprint = model.fingerprint_from_path(unpacked)
 
-    assert len(model_fingerprint["messages"]) == 2
-    assert len(model_fingerprint["stories"]) == 2
+    assert len(model_fingerprint["messages"]) == 3
+    assert len(model_fingerprint["stories"]) == 3
 
     domain_file = os.path.join(unpacked, "core", "domain.yml")
     domain = Domain.load(domain_file)
 
-    expected_intents = [
+    expected_intents = {
         "greet",
         "goodbye",
         "affirm",
         "deny",
         "mood_great",
         "mood_unhappy",
-    ]
+    }
 
     assert all([i in domain.intents for i in expected_intents])
 
+    nlu_training_data_file = os.path.join(unpacked, "nlu", "training_data.json")
+    nlu_training_data = RasaReader().read(nlu_training_data_file)
+
+    assert expected_intents == nlu_training_data.intents
+
     expected_actions = [
         "utter_greet",
         "utter_cheer_up",


Code review comments:
diff_hunk: @@ -23,6 +23,7 @@ Fixed
 - slack notifications from bots correctly render text
 - fixed usage of ``--log-file`` argument for ``rasa run`` and ``rasa shell``
 - check if correct tracker store is configured in local mode
+- loading of additional training data loading with the ``SkillSelector``
path: CHANGELOG.rst
text: @user1:
one `loading` too much :)