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

Policy cofiguration via yaml file #1169

Merged
merged 36 commits into from Oct 25, 2018

Conversation

Projects
None yet
4 participants
@twollnik
Copy link
Contributor

twollnik commented Oct 14, 2018

This PR fixes #1030

We can now have a policy_config.yaml such as:

policies:
- name: KerasPolicy
- name: FallbackPolicy
  intent_threshold: 0.3
  action_threshold: 0.4
- name: path.to.custom.policy.CustomPolicy
  arg1: val1
  arg2: val2

The name and location of the file containing this policy configuration information can be set with a cmd arg (--policy_config or -p). When the arg is not set, the Policies are set to be KerasPolicy, MemoizationPolicy and FallbackPolicy.

A few open questions from my side:

  1. does the load_from_yaml function belong in PolicyEnsemble? Should it rather be in utils, Agent or Policy?
  2. When both a policy_config.yaml and the cmd args specify values for the FallbackPolicy (such as nlu_threshold, ...) should the cmd arg or the value in the policy_config.yaml take precedence? Currently the value in policy_config.yaml takes precedence.
  3. I did not include the possibility of setting batch size and epochs in the policy_config. The way I see it there is no issue setting these values on the command line. What we currently lack is a way to add custom policies. Do you nevertheless prefer to have the possibility of setting these options in the policy_config.yaml?

Note: I did not yet have the time to test this significantyl. I opened this PR because I would like to get your input early on.

Looking forward to your thoughts and feedback.
Cheers!
Tom

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 14, 2018

CLA assistant check
All committers have signed the CLA.

@akelad

This comment has been minimized.

Copy link
Collaborator

akelad commented Oct 15, 2018

Thanks a lot for submitting this PR! @tmbo can you get back to Tom about those questions please?

@tmbo
Copy link
Member

tmbo left a comment

love the functionality - made a couple style comments, would be great if you could take a look so we can get this merged 🚀

Can you also add a test or two loading an example configuration?

concerning your questions:

  1. I'd split up the functionality. I think it makes sense to add a rasa_core/config.py, put the def load(...) in there and then in the policyensemble put a def from_yaml(cls, yaml)
  2. I think best would be to prefer args over config
  3. no, not necessary 👍

policies = []

for policy in config_data.get('policies'):

This comment has been minimized.

@tmbo

tmbo Oct 15, 2018

Member

.get('policies', []) or throw an explicit error peforehand that the policies section is missing, otherwise this will throw a None is not iterable error.


for policy in config_data.get('policies'):

module_name, _ , class_name = policy.get('name').rpartition('.')

This comment has been minimized.

@tmbo

tmbo Oct 15, 2018

Member

can we reuse utils.class_from_module_path(component_name) from rasa_nlu.utils?

This comment has been minimized.

@twollnik

twollnik Oct 17, 2018

Contributor

We can use the utils function if the policy path is fully specified. If the policy name is KerasPolicy we would need to import KerasPolicy in utils so that the name KerasPolicy is defined in globals(). This would lead to a circular import as Policy imports utils.

This comment has been minimized.

@tmbo

tmbo Oct 17, 2018

Member

I think all the policies are imported at package level so what should work is, within this function (not at the top of the file, but within the functions scope) do from rasa_core.policies import *

@@ -136,6 +137,12 @@ def create_argument_parser():
help="When a fallback is triggered (e.g. because the ML prediction "
"is of low confidence) this is the name of tje action that "
"will get triggered instead.")
parser.add_argument(
'-p', '--policy_config',

This comment has been minimized.

@tmbo

tmbo Oct 15, 2018

Member

to stay consistent with nlu I'd call this --config an d -c

@@ -146,6 +153,7 @@ def train_dialogue_model(domain_file, stories_file, output_path,
endpoints=AvailableEndpoints(),
max_history=None,
dump_flattened_stories=False,
policy_config=None,

This comment has been minimized.

@tmbo

tmbo Oct 15, 2018

Member

just config

This comment has been minimized.

@twollnik

twollnik Oct 23, 2018

Contributor

this isn't called config because of the naming conflict with config.py

This comment has been minimized.

@tmbo

tmbo Oct 24, 2018

Member

👍

MaxHistoryTrackerFeaturizer(BinarySingleStateFeaturizer(),
max_history=max_history))]
if policy_config is None:
policies = [

This comment has been minimized.

@tmbo

tmbo Oct 15, 2018

Member

can we put this into a separate function?

@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 17, 2018

@tmbo Thanks for your comments.
I can add a test, no problem.

@tmbo tmbo referenced this pull request Oct 19, 2018

Merged

Paper eval scripts #550

2 of 4 tasks complete
@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 23, 2018

@tmbo How can I get Travis to run the tests?

@tmbo
Copy link
Member

tmbo left a comment

Looking very good, some small suggestions left.

Please make sure to add the change to the Changelog.rst as well as adding it to the documentation in docs/.

@@ -0,0 +1,38 @@
{

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

do we need this model?

This comment has been minimized.

@twollnik

twollnik Oct 23, 2018

Contributor

No we don't. I am not sure how this was created.



def load(config_file, fallback_args, max_history):
# type: (Text, Dict, int) -> List[Policy]

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

Dict[Text, Any]

return PolicyEnsemble.from_dict(config_data)

def handle_precedence_and_defaults(config_data, fallback_args, max_history):
# type: (Dict, Dict, int) -> Dict

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

Dict[Text, Any]

@@ -194,6 +200,47 @@ def load(cls, path):
ensemble = ensemble_cls(policies, fingerprints)
return ensemble

@classmethod
def from_dict(cls, dict):

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

please rename dict, it is a builtin name

@@ -194,6 +200,47 @@ def load(cls, path):
ensemble = ensemble_cls(policies, fingerprints)
return ensemble

@classmethod
def from_dict(cls, dict):
# type: Dict -> List[Policy]

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

Dict[Text, Any]

@@ -81,6 +81,8 @@ def class_from_module_path(module_path):
import importlib

# load the module, will raise ImportError if module cannot be loaded
from rasa_core.policies import *

This comment has been minimized.

@tmbo

tmbo Oct 23, 2018

Member

please move this into PolicyEnsemble.from_dict - only relevant for that caller

@tmbo

This comment has been minimized.

Copy link
Member

tmbo commented Oct 23, 2018

@twollnik not sure what you mean, as long as your test is part of the tests directory, it should be run by travis

@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 23, 2018

@tmbo For me Travis is saying Expected - Waiting for status to be reported. The way I see it the tests are not beeing run. Or am I mistaken?

@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 23, 2018

@tmbo Could you have a look at the docs entry I wrote and make sure that it matches your usual format?

@akelad

This comment has been minimized.

Copy link
Collaborator

akelad commented Oct 24, 2018

@twollnik can you resolve the merge conflicts? that might be what's holding travis up

tmbo added some commits Oct 24, 2018

@tmbo

tmbo approved these changes Oct 24, 2018

Copy link
Member

tmbo left a comment

looks good - great job 🚀

I made some small improvements to the docs (moved them to a different part of the docs), will commit separately.

@@ -146,6 +153,7 @@ def train_dialogue_model(domain_file, stories_file, output_path,
endpoints=AvailableEndpoints(),
max_history=None,
dump_flattened_stories=False,
policy_config=None,

This comment has been minimized.

@tmbo

tmbo Oct 24, 2018

Member

👍

@tmbo

This comment has been minimized.

Copy link
Member

tmbo commented Oct 24, 2018

As a last step, we need to fix the travis errors. can you please take a look at

thanks a lot 🚀

@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 24, 2018

@tmbo Thanks for updating the docs and fixing merge conflicts!

twollnik and others added some commits Oct 24, 2018

@akelad

This comment has been minimized.

Copy link
Collaborator

akelad commented Oct 25, 2018

@twollnik just fixed the py2 test for you. once the build passes i'll merge

@tmbo tmbo merged commit d83693c into RasaHQ:master Oct 25, 2018

3 checks passed

codeclimate All good!
Details
coverage/coveralls Coverage decreased (-0.4%) to 78.396%
Details
license/cla Contributor License Agreement is signed.
Details
@tmbo

This comment has been minimized.

Copy link
Member

tmbo commented Oct 25, 2018

🚀 awesome job and thanks a lot for taking the time to finish this up, @twollnik

@twollnik

This comment has been minimized.

Copy link
Contributor

twollnik commented Oct 26, 2018

Glad to be able to help 😄
@tmbo @akelad Thank you for your commits and the feedback along the way!

@twollnik twollnik deleted the twollnik:policy_cofiguration_via_yaml branch Oct 26, 2018

@wochinge wochinge referenced this pull request Nov 1, 2018

Merged

Custom store endpoints #1256

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment