Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor interpreter usage in story visualization #9821

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Oct 7, 2021

Proposed changes:

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)

else:
message = el.parse_data
message[TEXT] = f"{INTENT_MESSAGE_PREFIX}{el.intent_name}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously (even on 2.8.x) it always used None for the nodes which is super bad UX

@@ -22,40 +21,6 @@ def test_load_training_data_reader_not_found_throws(tmp_path: Path, domain: Doma
training.load_data(str(tmp_path), domain)


async def test_story_visualization(domain: Domain, tmp_path: Path):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved them to the other visualization tests

Comment on lines 151 to 170
def test_story_visualization_with_processor(
domain: Domain, tmp_path: Path, default_agent: Agent
):
import rasa.shared.core.training_data.loading as core_loading

story_steps = core_loading.load_data_from_resource(
"data/test_yaml_stories/stories.yml", domain
)
out_file = tmp_path / "graph.html"
generated_graph = visualization.visualize_stories(
story_steps,
domain,
processor=default_agent.processor,
output_file=str(out_file),
max_history=3,
should_merge_nodes=False,
)

assert str(None) not in out_file.read_text()
assert "/affirm" in out_file.read_text()
assert len(generated_graph.nodes()) == 51
assert len(generated_graph.edges()) == 56


def test_story_visualization_with_training_data(
domain: Domain, tmp_path: Path, nlu_data_path: Text
):
import rasa.shared.core.training_data.loading as core_loading

story_steps = core_loading.load_data_from_resource(
"data/test_yaml_stories/stories.yml", domain
)
out_file = tmp_path / "graph.html"
test_text = "test text"
test_intent = "affirm"
generated_graph = visualization.visualize_stories(
story_steps,
domain,
output_file=str(out_file),
max_history=3,
should_merge_nodes=False,
nlu_training_data=TrainingData(
[Message({TEXT: test_text, INTENT: test_intent})]
),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added them to test whether things work as expected

@wochinge wochinge requested a review from joejuzl October 7, 2021 14:53
@wochinge wochinge marked this pull request as ready for review October 7, 2021 14:53
@wochinge wochinge requested a review from a team as a code owner October 7, 2021 14:53
@wochinge wochinge linked an issue Oct 7, 2021 that may be closed by this pull request
1 task
@wochinge wochinge force-pushed the origin/3.0-architecture-revamp/9766/story-visualization branch 2 times, most recently from 93e56ec to a98044e Compare October 7, 2021 16:05
@wochinge wochinge removed the request for review from a team October 8, 2021 07:23
@wochinge
Copy link
Contributor Author

wochinge commented Oct 8, 2021

I'll actually drop the processor. This is mostly related to Markdown. In practice there will always be an intent

@wochinge wochinge force-pushed the origin/3.0-architecture-revamp/9766/story-visualization branch from 1785aaf to 0d89bc6 Compare October 8, 2021 14:58
@wochinge wochinge requested a review from joejuzl October 8, 2021 14:58
@wochinge wochinge force-pushed the origin/3.0-architecture-revamp/9766/story-visualization branch from 0d89bc6 to fe32dfc Compare October 11, 2021 12:26
@wochinge wochinge force-pushed the origin/3.0-architecture-revamp/9766/story-visualization branch from fe32dfc to 0cc0ac9 Compare October 12, 2021 12:28
@wochinge
Copy link
Contributor Author

@joejuzl Do you have time to give this another review today?

@wochinge wochinge merged commit 204c1bb into 3.0-architecture-revamp/9277/recipe Oct 12, 2021
@wochinge wochinge deleted the origin/3.0-architecture-revamp/9766/story-visualization branch October 12, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp: Story visualisation.
2 participants