fix temporary files in 'rasa test' #8217

wochinge · 2021-03-16T15:01:39Z

Proposed changes:

fix During rasa test, YamlValidationException shows temp path #7640
introduce a new method TrainingDataImporter.get_conversation_tests which only retrieves conversation test files
change server to store test data in the expected format (tests directory for Markdown, test_ prefix for YAML)
use TrainingDataImporter to load data instead of moving eligble files to a separate temporary directory
added docstrings for importer package
renamed conversation test files to match expections (test_ prefix for YAML)

Status (please check what you already did):

added some tests for the functionality
updated the documentation
updated the changelog (please check changelog for instructions)
reformat files using black (please check Readme for instructions)

wochinge · 2021-03-16T18:34:56Z

rasa/shared/importers/rasa.py

@@ -54,10 +58,18 @@ async def get_stories(
            exclusion_percentage,
        )

+    async def get_conversation_tests(self) -> StoryGraph:
+        """Retrieves conversation test stories (see parent class for full docstring)."""


the alternative is to filter in get_stories based on use_e2e. Considering this and also to avoid anything breaking I decided to go for the explict extra method

wochinge · 2021-03-17T09:02:47Z

tests/conftest.py

@@ -138,7 +138,7 @@ def incorrect_nlu_data_path() -> Text:

 @pytest.fixture(scope="session")
 def end_to_end_story_path() -> Text:
-    return "data/test_evaluations/end_to_end_story.yml"
+    return "data/test_evaluations/test_end_to_end_story.yml"


test stories need to include the test_ prefix. Our tests were violating this.

joejuzl

Could you pinpoint for me the line which used to move the files to the temporary directory?

joejuzl · 2021-03-18T10:58:38Z

rasa/shared/importers/importer.py

+        Returns:
+            `StoryGraph` containing all loaded stories.
+        """
+        return await self.get_stories(use_e2e=True)


Why is this implemented in the interface?

This is implemented as fallback. Ideally they override this when they have a custom TrainingDataImporter class. We cannot let it raise in case it's not implemented as this might break existing implementations of custom TrainingDataImporters

rasa/shared/importers/rasa.py

joejuzl · 2021-03-18T11:10:22Z

rasa/core/test.py


    Args:
        model_dir: path to directory that contains the models to evaluate
        stories_file: path to the story file
        output: output directory to store results to
+        use_e2e: `True` if markdown story files should be parsed as end-to-end


This is not all it's doing right? It's also determining which files to load.

joejuzl · 2021-03-18T11:11:56Z

rasa/core/test.py

@@ -352,7 +352,11 @@ async def _create_data_generator(
    test_data_importer = TrainingDataImporter.load_from_dict(
        training_data_paths=[resource_name]
    )
-    story_graph = await test_data_importer.get_stories(use_e2e=use_e2e)
+    if use_e2e:


I think this branching is confusing, as we are also passing in a variable resource_name into the importer. It's hard to know how those two different things interplay. Would it not be clearer if we do this resource selection up higher when we start the test process, and then use_e2e could remain solely for determining how to read the file?

Very good point. The TrainingDataImporter is already categorizing files (e.g. into nlu and stories) and hence I think this should be part of the TrainingDataImporter. As this function _create_data_generator is called from different entrypoints I found it preferable to handle this in one place instead of multiple ones. Otherwise we might run into situations as we had it before where e.g. tests or also the server weren't respecting the conditions we have on these test files (e.g. the test_ prefix).

I agree that this is all confusing though 🙈 This still seemed the somewhat clearest approach to me though.

Maybe we should use a different name than use_e2e then? As it normally dictates what reader to use.
e.g. conversation_test_stories?

You mean renaming it within _create_data_generator or in general in the entire module? I'd be open to rename it for _create_data_generator but unfortunately use_e2e has multiple other implications on the testing

but unfortunately use_e2e has multiple other implications on the testing

This is why it might make sense to rename. You've added it to lots of methods in this PR with the sole aim of selecting different files in this method. Whereas it normally has the meaning of how a markdown file is read. When we remove markdown, and hopefully a lot of usages of use_e2e, we will still need this one to select conversation tests.

rasa/server.py

wochinge · 2021-03-18T12:57:16Z

rasa/cli/test.py

@@ -154,8 +152,12 @@ async def run_nlu_test_async(
        test_nlu,
    )

-    nlu_data = rasa.cli.utils.get_validated_path(data_path, "nlu", DEFAULT_DATA_PATH)
-    nlu_data = rasa.shared.data.get_nlu_directory(nlu_data)


@joejuzl This was moving NLU files to temporary directories

wochinge · 2021-03-18T12:57:30Z

rasa/cli/test.py

-    if args.e2e:
-        stories = rasa.shared.data.get_test_directory(stories)
-    else:
-        stories = rasa.shared.data.get_core_directory(stories)


@joejuzl This was moving Core files to temporary directories

joejuzl

Is there a reasonable way to test we are no longer creating the temp directories?

joejuzl · 2021-03-22T13:38:27Z

rasa/core/test.py

@@ -352,7 +352,11 @@ async def _create_data_generator(
    test_data_importer = TrainingDataImporter.load_from_dict(
        training_data_paths=[resource_name]
    )
-    story_graph = await test_data_importer.get_stories(use_e2e=use_e2e)
+    if use_e2e:


Maybe we should use a different name than use_e2e then? As it normally dictates what reader to use.
e.g. conversation_test_stories?

joejuzl · 2021-03-22T13:39:56Z

rasa/shared/importers/importer.py

@@ -48,16 +48,23 @@ async def get_stories(
        Returns:
            `StoryGraph` containing all loaded stories.
        """
-
+        # TODO: Drop `use_e2e` in Rasa Open Source 3.0.0 when removing Markdown support


This is confusing now that use_e2e also determines which files are loaded right?

I hope the renamed parameters make this less confusing. The importer only uses use_e2e for configuring the markdown reading. Means we no longer need it when Markdown is removed.

joejuzl · 2021-03-22T13:40:46Z

rasa/shared/importers/multi_project.py

        return self.config

    async def get_nlu_data(self, language: Optional[Text] = "en") -> TrainingData:
+        """Retrieves NLU training data (see parent class for full docstring)."""


Do we have a convention for docstrings on subclasses?

Not specifically. It's somewhat my personal preference to write a proper docstring in one place and then link to it (ideally I'd just omit but that doesn't work with pydocstyle)

rasa/shared/importers/rasa.py

wochinge · 2021-03-22T15:45:48Z

Is there a reasonable way to test we are no longer creating the temp directories?

I gave it a manual testing by putting a story with an invalid schema in the tests directory. I don't really fancy testing specific log message so opted for not testing this.

rasa/model_testing.py

tests/test_memory.py

tests/test_memory_joe.py

joejuzl · 2021-03-24T16:50:10Z

rasa/server.py

+        test_dir = temporary_directory / DEFAULT_CONVERSATION_TEST_PATH
+        test_dir.mkdir()
+        test_file = test_dir / f"tests{suffix}"
+        test_file.write_bytes(request.body)


Does this close the file after?

joejuzl

LGTM 👍

wochinge force-pushed the temp-filepaths-testing branch 6 times, most recently from 5fca09e to 1e3b6a0 Compare March 16, 2021 18:31

wochinge commented Mar 16, 2021

View reviewed changes

wochinge commented Mar 17, 2021

View reviewed changes

wochinge marked this pull request as ready for review March 17, 2021 12:50

wochinge requested review from a team and joejuzl and removed request for a team March 17, 2021 12:50

joejuzl suggested changes Mar 18, 2021

View reviewed changes

wochinge commented Mar 18, 2021

View reviewed changes

joejuzl suggested changes Mar 22, 2021

View reviewed changes

wochinge added 12 commits March 24, 2021 11:45

remove using temporary files for rasa test [nlu]

b1ac02f

add importer.get_conversation_tests method

856b41d

don't use temporary files for rasa test [core]

26cf5a1

update todos for dropping 'use_e2'

33371ec

improve use_e2e parameter naming for importers

12c55a2

add changelogs

128a85e

only use interface for end-to-end conversation tests

337d683

add required test_ prefix

a1e4e47

improve todo

153df97

fix story evaluation via server endpoint

2f225c4

remove ambiguous variable naming

94f32cb

remove special e2e handling from MultiprojectImporter

64856b0

wochinge force-pushed the temp-filepaths-testing branch from de10a14 to 64856b0 Compare March 24, 2021 10:47

wochinge requested a review from joejuzl March 24, 2021 10:47

joejuzl suggested changes Mar 24, 2021

View reviewed changes

wochinge added 2 commits March 24, 2021 17:53

delete unwanted files

687d88e

fix docstring

c8f9abb

wochinge requested a review from joejuzl March 24, 2021 16:56

joejuzl approved these changes Mar 24, 2021

View reviewed changes

wochinge enabled auto-merge March 24, 2021 17:07

wochinge merged commit 4b8f745 into main Mar 25, 2021

wochinge deleted the temp-filepaths-testing branch March 25, 2021 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix temporary files in 'rasa test' #8217

fix temporary files in 'rasa test' #8217

wochinge commented Mar 16, 2021 •

edited

Loading

wochinge Mar 16, 2021

wochinge Mar 17, 2021

joejuzl left a comment

joejuzl Mar 18, 2021

wochinge Mar 18, 2021

joejuzl Mar 18, 2021

joejuzl Mar 18, 2021

wochinge Mar 18, 2021

joejuzl Mar 22, 2021

wochinge Mar 22, 2021

joejuzl Mar 23, 2021

wochinge Mar 18, 2021

wochinge Mar 18, 2021

joejuzl left a comment

joejuzl Mar 22, 2021

joejuzl Mar 22, 2021

wochinge Mar 24, 2021

joejuzl Mar 22, 2021

wochinge Mar 22, 2021

wochinge commented Mar 22, 2021

joejuzl Mar 24, 2021

wochinge Mar 24, 2021

joejuzl left a comment

fix temporary files in 'rasa test' #8217

fix temporary files in 'rasa test' #8217

Conversation

wochinge commented Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joejuzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joejuzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wochinge commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joejuzl left a comment

Choose a reason for hiding this comment

wochinge commented Mar 16, 2021 •

edited

Loading