Check for missing MappingPolicy if triggers are defined #4147

msamogh · 2019-07-31T16:04:58Z

Proposed changes:

Throw error when triggers are defined in the domain, but there's no MappingPolicy defined in the configuration
Unify all checking of ensemble-domain incompatibilities in a single function
Part of Display warning about missing component/policy in config whenever possible #3886

Status (please check what you already did):

made PR ready for code review
added some tests for the functionality
updated the documentation
updated the changelog
reformat files using black (please check Readme for instructions)

erohmensing · 2019-07-31T17:59:57Z

since /restart is handled my the mappingpolicy, maybe we should also warn during run if the bot received a /restart intent but doesn't have mapping policy

msamogh · 2019-07-31T20:11:25Z

But that doesn't fall under things you can recognize during training time, right?

erohmensing · 2019-08-01T07:58:38Z

Yes that's true, we would have to handle it in run, there isn't a way to pre-warn. Was just an idea to give people more information about why the bot wasn't responding as expected, but perhaps oos of this PR

msamogh · 2019-08-01T08:40:30Z

@erohmensing Agreed. Although, I feel it'd be nicer if something as special as /restart was handled regardless of what policies are present.

wochinge

functionality looks great! I would just do a few improvements to the location of the methods and then we are ready to go.

rasa/core/agent.py

tests/core/test_agent.py

tests/core/conftest.py

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

wochinge

Great iteration! Looks really good now, couple of comments still

rasa/core/agent.py

rasa/core/domain.py

rasa/core/policies/ensemble.py

rasa/core/policies/form_policy.py

rasa/core/policies/mapping_policy.py

tests/core/test_agent.py

rasa/core/policies/ensemble.py

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

…into warn-missing-policy

rasa/core/policies/form_policy.py

rasa/core/policies/mapping_policy.py

rasa/core/domain.py

rasa/core/policies/ensemble.py

rasa/core/policies/mapping_policy.py

rasa/core/policies/two_stage_fallback.py

…into warn-missing-policy

wochinge

Looks great now! Also cool that you pulled it out of the agent module 🥇

rasa/core/domain.py

rasa/core/policies/form_policy.py

rasa/core/policies/mapping_policy.py

rasa/core/policies/two_stage_fallback.py

tests/core/test_agent.py

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Check for missing MappingPolicy if triggers are defined

f02df9e

msamogh force-pushed the warn-missing-policy branch from 87f635e to f02df9e Compare July 31, 2019 16:06

Fix test

ff223b7

msamogh marked this pull request as ready for review July 31, 2019 16:21

msamogh requested review from tmbo and wochinge July 31, 2019 16:21

Remove unused variables

60d6e53

Resolve merge conflicts

864cb05

msamogh force-pushed the warn-missing-policy branch from a61efb6 to 864cb05 Compare August 1, 2019 11:57

msamogh added 2 commits August 1, 2019 14:02

Update CHANGELOG

238804f

Merge branch 'master' into warn-missing-policy

cdb7a6b

wochinge suggested changes Aug 1, 2019

View reviewed changes

rasa/core/agent.py Outdated Show resolved Hide resolved

rasa/core/agent.py Outdated Show resolved Hide resolved

tests/core/test_agent.py Outdated Show resolved Hide resolved

tests/core/conftest.py Outdated Show resolved Hide resolved

msamogh and others added 11 commits August 2, 2019 09:58

Update rasa/core/agent.py

79a2dcd

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Add docstring

f3f3fa6

Move validation into respective Policy classes

57ec9d7

Merge

feced07

Add missing import

6f2ca47

Include test domain/config directly in unit test

4130087

Black formatting

b811cdc

Merge master

44eb68b

Remove unused from conftest

044fc27

Use YAML loading functions instead of file loading ones

3fab8b3

Black formatting

2153cd3

msamogh requested a review from wochinge August 6, 2019 11:14

wochinge reviewed Aug 6, 2019

View reviewed changes

Incorporate feedback

9eba573

msamogh and others added 4 commits August 7, 2019 12:13

Update rasa/core/policies/two_stage_fallback.py

98d76c4

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Accept None values for ensemble and domain

234abb0

Black formatting

61a4d18

Merge branch 'warn-missing-policy' of https://github.com/RasaHQ/rasa …

875e132

…into warn-missing-policy

wochinge reviewed Aug 8, 2019

View reviewed changes

msamogh added 12 commits August 8, 2019 14:58

Incoporate review feedback

2e2ff5a

remove file

7fe4d68

Split up into separate tests

71e8f29

Black formatting

3b5cbb9

Merge branch 'master' into warn-missing-policy

7f83361

Move imports inside function

b433ffc

Merge branch 'warn-missing-policy' of https://github.com/RasaHQ/rasa …

8d49ee7

…into warn-missing-policy

Remove unused import

672e92b

Use getattr to fix type error

f8a8725

Black formatting

a4ee411

Add import for type checking

ae0b631

Include more helpful exception message

0ac208d

msamogh requested a review from wochinge August 10, 2019 23:13

Fix formatted string

dd68010

wochinge approved these changes Aug 12, 2019

View reviewed changes

msamogh and others added 8 commits August 12, 2019 09:19

Update rasa/core/domain.py

094df7f

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/domain.py

2cd20e7

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/policies/form_policy.py

52dbc01

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/policies/two_stage_fallback.py

375df42

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/policies/two_stage_fallback.py

b2c8fb2

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/policies/form_policy.py

11bdcbc

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Update rasa/core/policies/mapping_policy.py

cd5ce4b

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>

Merge branch 'master' into warn-missing-policy

2c65947

msamogh merged commit 78001f2 into master Aug 12, 2019

msamogh deleted the warn-missing-policy branch August 12, 2019 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for missing MappingPolicy if triggers are defined #4147

Check for missing MappingPolicy if triggers are defined #4147

msamogh commented Jul 31, 2019 •

edited

erohmensing commented Jul 31, 2019

msamogh commented Jul 31, 2019

erohmensing commented Aug 1, 2019 •

edited

msamogh commented Aug 1, 2019

wochinge left a comment

wochinge left a comment

wochinge left a comment

Check for missing MappingPolicy if triggers are defined #4147

Check for missing MappingPolicy if triggers are defined #4147

Conversation

msamogh commented Jul 31, 2019 • edited

erohmensing commented Jul 31, 2019

msamogh commented Jul 31, 2019

erohmensing commented Aug 1, 2019 • edited

msamogh commented Aug 1, 2019

wochinge left a comment

Choose a reason for hiding this comment

wochinge left a comment

Choose a reason for hiding this comment

wochinge left a comment

Choose a reason for hiding this comment

msamogh commented Jul 31, 2019 •

edited

erohmensing commented Aug 1, 2019 •

edited