Adapted memoization policies for graph #9383
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twerkmeister
requested review from
a team,
koernerfelicia and
wochinge
and removed request for
a team
wochinge
reviewed
Aug 18, 2021
wochinge
reviewed
Aug 19, 2021
|
@twerkmeister Can we please also adapt the unit tests to the new GraphComponent versions of the policies? 🙌🏻 |
|
|
should also fix #5786 |
wochinge
reviewed
Aug 24, 2021
| @@ -123,6 +126,7 @@ def __init__( | |||
| featurizer: Optional[TrackerFeaturizer] = None, | |||
| ) -> None: | |||
| """Constructs a new Policy object.""" | |||
| self.config = config | |||
There was a problem hiding this comment.
?
Suggested change
| self.config = config | |
| self._config = config |
There was a problem hiding this comment.
@twerkmeister Seems this is still open?
There was a problem hiding this comment.
Yes indeed, I didn't do it here because it would mean another 100+ line changes because it would ripple through all the policies. In Ted policy alone there are like 50 accesses to self.config. Let's do it in a focused, separate PR just after this to not clutter this one here?
joejuzl
suggested changes
Aug 30, 2021
Co-authored-by: Joe Juzl <joejuzl@gmail.com>
aeshky
added a commit
that referenced
this pull request
Sep 6, 2021
…https://github.com/RasaHQ/rasa into 3.0-architecture-revamp/9330/NLUTrainingDataProvider * '3.0-architecture-revamp/9330/NLUTrainingDataProvider' of https://github.com/RasaHQ/rasa: Ignore assessed vulnerabilities Adapted memoization policies for graph (#9383)
ErickGiffoni
pushed a commit
to FGA-GCES/rasa
that referenced
this pull request
Sep 9, 2021
* Adapted memoization policies for graph * Not persisting config anymore for graph policy components * adjusted unit testing for memoization graph component * Removed max history hack in ted policy * Making sure policy config is used for initializing featurizers if necessary * Added policy priority to ted policy default config * Simplified policy test suites * Persisting policy by default when training * Only carrying over max_history from policy to featurizer for MaxHistoryTrackerFeaturizers * Turned load fail logging to warning from info * Removed compatibility code for old policy component types * Removed metadata and persist abstract methods from policy class * Joined all memoization policy tests * Fixed delorean code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes:
MemoizationPolicyandAugmentedMemoizationPolicyto newGraphComponentinterface #9344important context: https://www.notion.so/rasa/Migrating-policies-to-the-new-model-architecture-819812d37d784e4d98347d74ea4844fc#369e558d39b74a5096081a0408c23665
Status (please check what you already did):
black(please check Readme for instructions)