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.

Sign up for GitHub

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

Jump to bottom

implement DefaultV1 Recipe #9403

Closed
wants to merge 3 commits into from
Closed

implement DefaultV1 Recipe #9403

wants to merge 3 commits into from

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Aug 19, 2021 •
edited
Loading

Proposed changes:

  • fix Implement Recipe to convert current config to GraphSchema #9277
  • implement recipe which converts current config to graph schemas
  • logic to convert regular configs to train and predict graph schemas
  • interpolates passed CLI params in to the graph components config
  • handles Core only / NLU only training
  • implemented decorator which allows to register classes with the DefaultV1Recipe

Open Todos

  • wait until all graph components have been adapted to GraphComponent interface and then register all these classes with the decorator
  • change the current isinstance checks to check the component type via the registered ComponentType

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
Copy link
Contributor Author

@joejuzl that's what I have so far

@wochinge wochinge linked an issue Aug 23, 2021 that may be closed by this pull request
Implement Recipe to convert current config to GraphSchema #9277
Closed
4 tasks
@wochinge wochinge force-pushed the 3.0-architecture-revamp/9277/recipe branch 3 times, most recently from 655ecfb to ed60bf9 Compare August 23, 2021 16:48
@wochinge
Copy link
Contributor Author

wochinge commented Aug 25, 2021 •
edited
Loading

The new edge case about "rule_only_data" would still need to be considered (see here) ✅

@joejuzl
Copy link
Contributor

joejuzl commented Sep 1, 2021 •
edited by wochinge
Loading

@wochinge reminder: JiebaTokenizer persists and loads. (Although we may be able to avoid it.) ✅

@wochinge
Copy link
Contributor Author

wochinge commented Sep 7, 2021

reminder to pass language to Duckings model config if ducklings model config is not filled

@wochinge
Copy link
Contributor Author

wochinge commented Sep 13, 2021 •
edited
Loading

EntitySynonymMapper doesn't add anything during training which is required by subsequent components ✅

@wochinge wochinge force-pushed the 3.0-architecture-revamp/9277/recipe branch 4 times, most recently from f24b4a0 to c9a9f8d Compare September 15, 2021 15:42
wochinge
wochinge commented Sep 15, 2021
View reviewed changes
rasa/core/policies/_ensemble.py
@@ -74,6 +72,8 @@ def __init__(
self._set_rule_only_data()

def _set_rule_only_data(self) -> None:
from rasa.core.policies.rule_policy import RulePolicy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes here were needed to avoid cyclic imports

@wochinge wochinge force-pushed the 3.0-architecture-revamp/9277/recipe branch from c9a9f8d to 5a1a964 Compare September 15, 2021 15:45
@wochinge wochinge force-pushed the 3.0-architecture-revamp/9277/recipe branch from 5a1a964 to 71d23b8 Compare September 16, 2021 11:11
@wochinge wochinge closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Recipe to convert current config to GraphSchema
2 participants
@wochinge @joejuzl