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

IntentTEDPolicy: rasa interactive updates #8906

Merged
merged 10 commits into from
Jun 23, 2021

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Jun 17, 2021

Proposed changes:

It requires rasa test updates to be merged first! #8843

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)

@alwx alwx changed the base branch from main to rasa-train-intent-ted June 17, 2021 08:06
@alwx
Copy link
Contributor Author

alwx commented Jun 17, 2021

There are literally no tests for rasa interactive and _validate_action function that's why nothing was added here as well. Probably the right action would be to add an issue to add tests for rasa interactive.

@alwx alwx requested review from wochinge and ancalita June 17, 2021 08:27
@alwx alwx marked this pull request as ready for review June 17, 2021 08:27
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

I know it's a hard to test module but could we please try to add a test for the new edge case?

rasa/core/training/interactive.py Outdated Show resolved Hide resolved
rasa/core/training/interactive.py Outdated Show resolved Hide resolved
rasa/core/training/interactive.py Outdated Show resolved Hide resolved
@alwx alwx requested review from wochinge and ancalita June 21, 2021 11:23
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Can we please add a test for it, Alex?

@alwx alwx requested review from wochinge and TyDunn June 22, 2021 12:37
@alwx alwx force-pushed the rasa-interactive-intent-ted branch from 441cb4f to c2e13b6 Compare June 22, 2021 13:10
tests/core/training/test_interactive.py Show resolved Hide resolved
tests/core/training/test_interactive.py Outdated Show resolved Hide resolved
tests/core/training/test_interactive.py Outdated Show resolved Hide resolved
)
monkeypatch.setattr(interactive, "_form_is_rejected", Mock(return_value=False))
monkeypatch.setattr(interactive, "_form_is_restored", Mock(return_value=False))
monkeypatch.setattr(interactive, "send_action", asyncio.coroutine(Mock()))
Copy link
Contributor

Choose a reason for hiding this comment

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

how about making sure this gets called with the expected values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@alwx alwx requested a review from wochinge June 23, 2021 09:55
f"to indicate that the last user message was unexpected "
f"at this point in the conversation. "
f"Check out IntentTEDPolicy ({DOCS_URL_POLICIES}/#intent-ted-policy) "
f"to learn more."
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to ask the user here to press Y/N to continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what we've decided to do, yes (to keep it consistent with the rest)

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, then maybe it should be explicitly stated? Unless questionary takes care of this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO this should instead be written in docs with the explanation what action_unlikely_intent is and how it works. We can also write it here but I am afraid the message will be too long in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I meant maybe it's not obvious what keys to press from the question and that should be explicitly stated, thats all 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, got it! Actually, questionary handles that and shows the available options automatically :)

is_correct = await _ask_questions(question, conversation_id, endpoint)
is_correct = (
await _ask_questions(question, conversation_id, endpoint)
or action_name == ACTION_UNLIKELY_INTENT_NAME
Copy link
Member

Choose a reason for hiding this comment

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

Does this line imply that regardless of user answer to the query, the action is always correct for action_unlikely_intent?

Copy link
Contributor Author

@alwx alwx Jun 23, 2021

Choose a reason for hiding this comment

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

It means that the choice (Y/N) doesn't affect what's gonna be happening next.

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test! 💯

@alwx alwx merged commit e0794c0 into rasa-train-intent-ted Jun 23, 2021
@alwx alwx deleted the rasa-interactive-intent-ted branch June 23, 2021 11:55
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.

IntentTEDPolicy: Adapt rasa interactive
4 participants