Merge pull request RasaHQ#5122 from RasaHQ/issue4896

for `rasa test core --evaluate-model-directory` set default for `--model` to directory

Author: chkoss

changelog/4896.bugfix.rst

@@ -0,0 +1,3 @@
+Fixed default behavior of ``rasa test core --evaluate-model-directory`` when called without ``--model``. Previously, the latest model file was used as ``--model``. Now the default model directory is used instead.
+
+New behavior of ``rasa test core --evaluate-model-directory`` when given an existing file as argument for ``--model``: Previously, this led to an error. Now a warning is displayed and the directory containing the given file is used as ``--model``.

rasa/test.py

@@ -2,6 +2,7 @@
 import logging
 import os
 from typing import Text, Dict, Optional, List, Any
+from pathlib import Path
 
 import rasa.utils.io as io_utils
 from rasa.constants import (
@@ -16,9 +17,13 @@
 logger = logging.getLogger(__name__)
 
 
-def test_core_models_in_directory(model_directory: Text, stories: Text, output: Text):
+def test_core_models_in_directory(
+    model_directory: Text, stories: Text, output: Text
+) -> None:
     from rasa.core.test import compare_models_in_dir, plot_core_results
 
+    model_directory = _get_sanitized_model_directory(model_directory)
+
     loop = asyncio.get_event_loop()
     loop.run_until_complete(compare_models_in_dir(model_directory, stories, output))
 
@@ -27,6 +32,31 @@ def test_core_models_in_directory(model_directory: Text, stories: Text, output:
     plot_core_results(output, number_of_stories)
 
 
+def _get_sanitized_model_directory(model_directory: Text) -> Text:
+    """Adjusts the `--model` argument of `rasa test core` when called with `--evaluate-model-directory`.
+
+    By default rasa uses the latest model for the `--model` parameter. However, for `--evaluate-model-directory` we
+    need a directory. This function checks if the passed parameter is a model or an individual model file.
+
+    Args:
+        model_directory: The model_directory argument that was given to `test_core_models_in_directory`.
+
+    Returns:
+        The adjusted model_directory that should be used in `test_core_models_in_directory`.
+    """
+    import rasa.model
+
+    p = Path(model_directory)
+    if p.is_file():
+        if model_directory != rasa.model.get_latest_model():
+            print_warning(
+                "You passed a file as '--model'. Will use the directory containing this file instead."
+            )
+        model_directory = str(p.parent)
+
+    return model_directory
+
+
 def test_core_models(models: List[Text], stories: Text, output: Text):
     from rasa.core.test import compare_models
 

tests/test_test.py

@@ -0,0 +1,66 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch) -> None:
+    latest_model = tmp_path / "my_test_model.tar.gz"
+    monkeypatch.setattr(rasa.model, "get_latest_model", lambda: str(latest_model))
+
+
+def test_get_sanitized_model_directory_when_not_passing_model(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _get_sanitized_model_directory
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    # Create a fake model on disk so that `is_file` returns `True`
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    new_modeldir = _get_sanitized_model_directory(str(latest_model))
+    captured = capsys.readouterr()
+    assert not captured.out
+    assert new_modeldir == str(latest_model.parent)
+
+
+def test_get_sanitized_model_directory_when_passing_model_file_explicitly(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _get_sanitized_model_directory
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    other_model = tmp_path / "my_test_model1.tar.gz"
+    assert str(other_model) != rasa.model.get_latest_model()
+    other_model.touch()
+
+    # Input: some file
+    # => Should return containing directory and print a warning
+    new_modeldir = _get_sanitized_model_directory(str(other_model))
+    captured = capsys.readouterr()
+    assert captured.out
+    assert new_modeldir == str(other_model.parent)
+
+
+def test_get_sanitized_model_directory_when_passing_other_input(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _get_sanitized_model_directory
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    # Input: anything that is not an existing file
+    # => Should return input
+    modeldir = "random_dir"
+    assert not Path(modeldir).is_file()
+    new_modeldir = _get_sanitized_model_directory(modeldir)
+    captured = capsys.readouterr()
+    assert not captured.out
+    assert new_modeldir == modeldir