Skip to content
This repository has been archived by the owner on Aug 22, 2019. It is now read-only.

Tracker_store added in endpoints.yml #1048

Merged
merged 30 commits into from Oct 31, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
7593507
Added tracker_store to endpoints.yml
souvikg10 Sep 22, 2018
962dc43
Added example for tracker_store endpoint.yml
souvikg10 Sep 22, 2018
83b3e9b
Merge branch 'master' into master
MetcalfeTom Oct 10, 2018
523cfb2
Merge new pull, fixed conflicts on tracker store.py
souvikg10 Oct 12, 2018
859836b
Merge branch 'master' into master
souvikg10 Oct 12, 2018
e00f7ac
FIX tracker_store to be read from endpoints
souvikg10 Oct 12, 2018
39e8302
Merge branch 'master' of https://github.com/souvikg10/rasa_core
souvikg10 Oct 12, 2018
57d26d3
Change argument for tracker store to kwargs
souvikg10 Oct 17, 2018
bf1b532
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 17, 2018
d12d1de
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 19, 2018
d42fd37
Format fixes and updated utils.py endpoint configuration
souvikg10 Oct 19, 2018
771ff80
Updated example endpoint
souvikg10 Oct 19, 2018
be75b1c
Fixes on endpoint config
souvikg10 Oct 19, 2018
b6d3dd9
Merge branch 'master' into master
akelad Oct 23, 2018
34c46d9
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 23, 2018
0cd8e78
pep8 fixes, added tests for tracker_store
souvikg10 Oct 23, 2018
abee451
Merge branch 'master' of https://github.com/souvikg10/rasa_core
souvikg10 Oct 23, 2018
477108a
fixed pep8 length issues
souvikg10 Oct 23, 2018
b437f9b
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 25, 2018
11a0b0e
fix length issues
souvikg10 Oct 25, 2018
3d1a378
Added defaults for tracker_store params, updated changelog
souvikg10 Oct 25, 2018
48b74b3
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 25, 2018
0d8db77
Updated default tracker_store variables, changed timeout to record_exp
souvikg10 Oct 26, 2018
0b35da7
fix pep8 length issues
souvikg10 Oct 26, 2018
3a3241d
changed method to static
souvikg10 Oct 26, 2018
9bd7a09
Updated command for static method of tracker_store
souvikg10 Oct 26, 2018
d46d480
Setting domain to None and removing template domain dependencies
souvikg10 Oct 29, 2018
123fefe
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 29, 2018
dfeb3c3
changed else to elif
souvikg10 Oct 30, 2018
de4696c
Merge branch 'master' of https://github.com/RasaHQ/rasa_core
souvikg10 Oct 30, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion data/example_endpoints.yml
Expand Up @@ -7,4 +7,17 @@ nlg:
# basic_auth:
# username: user
# password: pass

# example of redis external tracker store config
tracker_store:
store_type: redis
url: http://localhost
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be url: localhost (omitting the http://)
It would also be great if you include port: 6379 in your example 😊.

db: 0
password: password
timeout: 30000
# example of mongoDB external tracker store config
#tracker_store:
#store_type: mongod
#url: mongodb://localhost:27017
#db: rasa
#user: username
#password: password
5 changes: 2 additions & 3 deletions rasa_core/agent.py
Expand Up @@ -32,7 +32,7 @@
from rasa_core.policies.ensemble import SimplePolicyEnsemble, PolicyEnsemble
from rasa_core.policies.memoization import MemoizationPolicy
from rasa_core.processor import MessageProcessor
from rasa_core.tracker_store import InMemoryTrackerStore, TrackerStore
from rasa_core.tracker_store import InMemoryTrackerStore
from rasa_core.trackers import DialogueStateTracker, EventVerbosity
from rasa_core.utils import EndpointConfig
from rasa_nlu.utils import is_url
Expand Down Expand Up @@ -648,8 +648,7 @@ def create_tracker_store(store, domain):
store.domain = domain
return store
else:
return InMemoryTrackerStore(domain)

return InMemoryTrackerStore(domain)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a blank line after the method

@staticmethod
def _create_ensemble(
policies # type: Union[List[Policy], PolicyEnsemble, None]
Expand Down
11 changes: 7 additions & 4 deletions rasa_core/run.py
Expand Up @@ -5,7 +5,7 @@

from builtins import str
from collections import namedtuple

import os
import argparse
import logging
from flask import Flask
Expand All @@ -23,8 +23,9 @@
BUILTIN_CHANNELS)
from rasa_core.interpreter import (
NaturalLanguageInterpreter)
from rasa_core.tracker_store import TrackerStore
from rasa_core.utils import read_yaml_file, AvailableEndpoints

from rasa_core.domain import TemplateDomain
logger = logging.getLogger() # get the root logger


Expand Down Expand Up @@ -217,7 +218,7 @@ def load_agent(core_model, interpreter, endpoints,
generator=endpoints.nlg,
action_endpoint=endpoints.action,
model_server=endpoints.model,
tracker_store=tracker_store,
tracker_store=endpoints.tracker_store,
wait_time_between_pulls=wait_time_between_pulls
)
else:
Expand Down Expand Up @@ -245,10 +246,12 @@ def load_agent(core_model, interpreter, endpoints,
_endpoints = AvailableEndpoints.read_endpoints(cmdline_args.endpoints)
_interpreter = NaturalLanguageInterpreter.create(cmdline_args.nlu,
_endpoints.nlu)
domain = TemplateDomain.load(os.path.join(cmdline_args.core, "domain.yml"))
Copy link
Member

Choose a reason for hiding this comment

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

this file might not exist: the agent might be loaded from a remote server and this directory might be empty

It is fine to set the TrackerStore's domain to None in line L250 and not load the domain here (the actual domain will be updated once it is loaded) - though we really need to fix this...

_tracker_store = TrackerStore(domain).find_tracker_store(_endpoints.tracker_store)
_agent = load_agent(cmdline_args.core,
interpreter=_interpreter,
tracker_store=_tracker_store,
endpoints=_endpoints)

serve_application(_agent,
cmdline_args.connector,
cmdline_args.port,
Expand Down
10 changes: 9 additions & 1 deletion rasa_core/tracker_store.py
Expand Up @@ -30,7 +30,14 @@ def __init__(self, domain, event_broker=None):
# type: (Optional[Domain], Optional[EventChannel]) -> None
self.domain = domain
self.event_broker = event_broker

Copy link
Contributor

Choose a reason for hiding this comment

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

please re-add an empty line here

def find_tracker_store(self, store=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that this should be class method. We are just using the domain attribute which could just be a parameter of find_tracker_store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, you don't need to instantiate the TrackerStore class in order to retrieve a tracker_store class. I can change that to return a cls object

if store is None:
souvikg10 marked this conversation as resolved.
Show resolved Hide resolved
return InMemoryTrackerStore(self.domain)
elif store.store_type == 'redis':
return RedisTrackerStore(domain=self.domain,host=store.url,db=store.db,password=store.password,record_exp=store.timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if the user could also define a certain port through the configuration file.
Hence, we should probably change this line to:

return RedisTrackerStore(domain=self.domain, 
                         host=store.url, 
                         port=store.port,
                         db=store.db, 
                         password=store.password, 
                         record_exp=store.timeout)

elif store.store_type == 'mongod':
return MongoTrackerStore(domain=self.domain,host=store.url,db=store.db,username=store.user,password=store.password)

def get_or_create_tracker(self, sender_id):
tracker = self.retrieve(sender_id)
if tracker is None:
Expand Down Expand Up @@ -124,6 +131,7 @@ def __init__(self, domain, host='localhost',
port=6379, db=0, password=None, event_broker=None,
record_exp=None):


Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the empty line

import redis
self.red = redis.StrictRedis(host=host, port=port, db=db,
password=password)
Expand Down
7 changes: 5 additions & 2 deletions rasa_core/train.py
Expand Up @@ -21,7 +21,8 @@
from rasa_core.policies.memoization import MemoizationPolicy
from rasa_core.run import AvailableEndpoints
from rasa_core.training import interactive

from rasa_core.tracker_store import TrackerStore
from rasa_core.domain import TemplateDomain
logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -216,7 +217,8 @@ def train_dialogue_model(domain_file, stories_file, output_path,
_endpoints = AvailableEndpoints.read_endpoints(cmdline_args.endpoints)
_interpreter = NaturalLanguageInterpreter.create(cmdline_args.nlu,
_endpoints.nlu)

domain = TemplateDomain.load(cmdline_args.domain)
_tracker_store = TrackerStore(domain).find_tracker_store(_endpoints.tracker_store)
Copy link
Contributor

Choose a reason for hiding this comment

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

please insert an empty line here for better readability

if cmdline_args.core:
if not cmdline_args.interactive:
raise ValueError("--core can only be used together with the"
Expand All @@ -230,6 +232,7 @@ def train_dialogue_model(domain_file, stories_file, output_path,
_agent = Agent.load(cmdline_args.core,
interpreter=_interpreter,
generator=_endpoints.nlg,
tracker_store=_tracker_store,
action_endpoint=_endpoints.action)
else:
if not cmdline_args.out:
Expand Down
20 changes: 16 additions & 4 deletions rasa_core/utils.py
Expand Up @@ -595,27 +595,34 @@ def read_endpoints(cls, endpoint_file):
endpoint_file, endpoint_type="action_endpoint")
model = read_endpoint_config(
endpoint_file, endpoint_type="models")
tracker_store = read_endpoint_config(endpoint_file, endpoint_type="tracker_store")

return cls(nlg, nlu, action, model)
return cls(nlg, nlu, action, model, tracker_store)

def __init__(self, nlg=None, nlu=None, action=None, model=None):
def __init__(self, nlg=None, nlu=None, action=None, model=None,tracker_store=None):
self.model = model
self.action = action
self.nlu = nlu
self.nlg = nlg
self.tracker_store = tracker_store


class EndpointConfig(object):
"""Configuration for an external HTTP endpoint."""

def __init__(self, url, params=None, headers=None, basic_auth=None,
token=None, token_name="token"):
token=None, token_name="token",store_type=None,db=None,user=None,password=None,timeout=None):
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we should set all of them as separate arguments. Now that we start to use this for more purposes, I think it makes sense to just take all the **kwargs and store them on the objects property dict:

for k, v in kwargs.items():
  setattr(self, k, v)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah makes sense, i had some thoughts on that as well. I will try to do that next week 👍

self.url = url
self.params = params if params else {}
self.headers = headers if headers else {}
self.basic_auth = basic_auth
self.token = token
self.token_name = token_name
self.store_type = store_type
self.db = db
self.user = user
self.password= password
self.timeout = timeout

def request(self,
method="post", # type: Text
Expand Down Expand Up @@ -672,7 +679,12 @@ def from_dict(cls, data):
data.get("headers"),
data.get("basic_auth"),
data.get("token"),
data.get("token_name"))
Copy link
Contributor

Choose a reason for hiding this comment

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

you have either to pass data (as it is a dictionary) or you have to assign the entries to keys, e.g.

[...],
token_name=data.get('token_name'),
store_type=data.get('store_type'),
[...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I noticed the issue comes when I added **kwargs so each parameter became a key value pair, I think it is also best here to change the key value pair as well. Sorry overlooked it during the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I noticed the issue comes when I added **kwargs so each parameter became a key value pair, I think it is also best here to change the key value pair as well. Sorry overlooked it during the tests

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively:

EndpointConfig(
                data.get("url"),
                data.get("params"),
                data.get("headers"),
                data.get("basic_auth"),
                data.get("token"),
                data.get("token_name"),
                **data)

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 makes sense. I will do the fixes this weekend 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your efforts! Your feature will help a lot of users in the future to use different tracker stores 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the issues, however i haven't found any formatting error in Travis, not sure how to check that though

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I will have another look on them later. Regarding travis: I think we have to let it built first. This takes usually about 20 minutes after you have pushed your changes.

Copy link
Contributor

@wochinge wochinge Oct 19, 2018

Choose a reason for hiding this comment

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

The parameters are overlapping here. See https://travis-ci.com/RasaHQ/rasa_core/jobs/152986926 line 1495.

A solution to this could be:

return EndpointConfig(**data)

and a constructor like this:

class EndpointConfig(object):
    """Configuration for an external HTTP endpoint."""

    def __init__(self, url, params=None, headers=None, basic_auth=None,
                 token=None, token_name="token",**kwargs):
        self.url = url
        self.params = params if params else {}
        self.headers = headers if headers else {}
        self.basic_auth = basic_auth
        self.token = token
        self.token_name = token_name

        [setattr(self, k, v) for k, v in kwargs.items() if not hasattr(self, k)]

I used the if not hasattr(self, k) to not overwrite the dictionaries of self.params and self.headers with None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the formatting issues: You can also check that locally, e.g. by using a linter which knows pep8 or IDEs such as PyCharm or Visual Studio Code (with the Python plugin).

data.get("token_name"),
data.get("store_type"),
data.get("db"),
data.get("user"),
data.get("password"),
data.get("timeout"))

def __eq__(self, other):
if isinstance(self, type(other)):
Expand Down