Max_history parameter should be carried over to the featurizer definition in TEDPolicy

[akelad]

If you have your configuration defined like this, with the featurizer defined explicitly:

• name: TEDPolicy
  max_history: 5
  featurizer:
• name: MaxHistoryTrackerFeaturizer
  state_featurizer:
• name: LabelTokenizerSingleStateFeaturizer

The max_history parameter won't get carried over to the MaxHistoryTrackerFeaturizer. This is kind of strange behaviour, because if you specify the policy like this:

• name: TEDPolicy
  max_history: 5

The policy loads up MaxHistoryTrackerFeaturizer internally with max_history: 5.

See forum post for source of confusion: https://forum.rasa.com/t/running-oom-in-tedpolicy-with-900-stories-1-10-0/28230/6

[chkoss]

Exalate commented:

chkoss commented:

Is this rather an issue for @RasaHQ/production-squad or for research squad?

[wochinge]

Exalate commented:

wochinge commented:

@Ghostvv Does it actually make sense to do this when we use Genie's featurizer for everything? If this should still be done, then I think we can handle this + I think it's even good if we can get our feet wet with easier issues like this one.

[Ghostvv]

Exalate commented:

Ghostvv commented:

it still makes sense to do, we'll have max_history parameter anyhow

[joaoCeilandia]

Exalate commented:

joaoCeilandia commented:

Me and some friends where working on this issue and we found that this behavior happens because when creating a featurizer it is instantiated as a TrackerFeaturizer and when no featurizer is instantiated, it is created as a MaxHistoryTrackerFeaturizer. this code shows this:

So the only way to make max_history be inside the featurizer is by don't instantiating one.

we tought about make a change by our own to try to fix this but as we don't know what is wanted for this issue( e.g. if we could make a non-instanciated featurizer just became a TrackerFeaturizer or make a intanciated one became a MaxHistoryFeaturizer ), we came ask for some directioning.

@akelad @Ghostvv @wochinge @chkoss

[dakshvar22]

Exalate commented:

dakshvar22 commented:

@wochinge IMO, the fix here does not solve the problem correctly but is more of a hack. It passes the specified max_history parameter to the policy and relies on the policy to set the max_history of tracker. An easier and more full proof fix of this would be to do this inside get_featurizer_from_dict function in ensemble.py. The real issue is that get_featurizer_from_dict assumes that the dictionary (policy) passed to it contains information for all attributes including max_history. However, for that assumption to be correct from_dict function should add the max_history parameter if specified to the dictionary constructed here. This would simplify the constructor and load methods of policies. What do you think? Should I reopen this issue and put it in enable's inbox?

[wochinge]

Exalate commented:

wochinge commented:

However, for that assumption to be correct from_dict function should add the max_history parameter if specified to the dictionary constructed here.

You mean using something like setdefault to avoid overriding any given max_history param? Makes absolutely sense for me 👍🏻

[dakshvar22]

Exalate commented:

dakshvar22 commented:

Also, please let research team know once the issue is closed as there are[ bits of code in policies|

rasa/rasa/core/policies/ted_policy.py

Line 336 in 4b06e36

# More context here - https://github.com/RasaHQ/rasa/issues/5786#issuecomment-840762751

] which will simplify once this is in place.

[stale]

Exalate commented:

stale[bot] commented:

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sync-by-unito]

➤ Maxime Verger commented:

💡 Heads up! We're moving issues to Jira: https://rasa-open-source.atlassian.net/browse/OSS.

From now on, this Jira board is the place where you can browse (without an account) and create issues (you'll need a free Jira account for that). This GitHub issue has already been migrated to Jira and will be closed on January 9th, 2023. Do not forget to subscribe to the corresponding Jira issue!

➡️ More information in the forum: https://forum.rasa.com/t/migration-of-rasa-oss-issues-to-jira/56569.