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

Tracker_store added in endpoints.yml #1048

Merged
merged 30 commits into from Oct 31, 2018

Conversation

Projects
None yet
5 participants
@souvikg10
Copy link
Contributor

souvikg10 commented Sep 22, 2018

Proposed changes:

  • A way to externalise the tracker_store from default(InMemory) to either Redis or MongoDB using the endpoints.yml
    You can add the config for the Redis or MongoDB in the endpoints.yml file, mentioning the store_type either 'redis' or 'mongod'

This will help not to add new code in order to externalise the tracker store

Issue #1035

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
@akelad

This comment has been minimized.

Copy link
Collaborator

akelad commented Sep 24, 2018

Thanks for submitting the PR, we'll review it as soon as possible 🙂

@tmbo tmbo requested review from tmbo and MetcalfeTom Oct 4, 2018

@MetcalfeTom

This comment has been minimized.

Copy link
Contributor

MetcalfeTom commented Oct 12, 2018

Love the functionality! Will require a little more implementation - please check out the store.store_type error raised by Travis here

@souvikg10

This comment has been minimized.

Copy link
Contributor

souvikg10 commented Oct 12, 2018

I wonder why this is throwing an error though for InMemoryTracker, I didn't change anything for that

@souvikg10

This comment has been minimized.

Copy link
Contributor

souvikg10 commented Oct 12, 2018

Nevermind, i see the issue 🗡 , I will update it

souvikg10 added some commits Oct 12, 2018



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):

This comment has been minimized.

@tmbo

tmbo Oct 12, 2018

Member

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)

This comment has been minimized.

@souvikg10

souvikg10 Oct 12, 2018

Contributor

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

@@ -648,8 +648,7 @@ def create_tracker_store(store, domain):
store.domain = domain
return store
else:
return InMemoryTrackerStore(domain)

return InMemoryTrackerStore(domain)

This comment has been minimized.

@wochinge

wochinge Oct 18, 2018

Contributor

please add a blank line after the method

@@ -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

This comment has been minimized.

@wochinge

wochinge Oct 18, 2018

Contributor

please re-add an empty line here

@@ -124,6 +131,7 @@ def __init__(self, domain, host='localhost',
port=6379, db=0, password=None, event_broker=None,
record_exp=None):


This comment has been minimized.

@wochinge

wochinge Oct 18, 2018

Contributor

please remove the empty line

@@ -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)

This comment has been minimized.

@wochinge

wochinge Oct 18, 2018

Contributor

please insert an empty line here for better readability

@@ -678,7 +681,12 @@ def from_dict(cls, data):
data.get("headers"),
data.get("basic_auth"),
data.get("token"),
data.get("token_name"))

This comment has been minimized.

@wochinge

wochinge Oct 18, 2018

Contributor

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'),
[...]

This comment has been minimized.

@souvikg10

souvikg10 Oct 18, 2018

Contributor

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

This comment has been minimized.

@souvikg10

souvikg10 Oct 18, 2018

Contributor

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

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

alternatively:

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

This comment has been minimized.

@souvikg10

souvikg10 Oct 19, 2018

Contributor

that makes sense. I will do the fixes this weekend 👍

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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

This comment has been minimized.

@souvikg10

souvikg10 Oct 19, 2018

Contributor

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

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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.

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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.

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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).

@wochinge

This comment has been minimized.

Copy link
Contributor

wochinge commented Oct 18, 2018

If you fix the error in rasa_core.utils.py, fix the few formatting issues and add a few test we should be good to go with your pull request! 🚀 I didn't comment on every formatting issue, Travis should give you feedback on that 😄
Thanks for your effort!

if store is None:
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)

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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)
# example of redis external tracker store config
tracker_store:
store_type: redis
url: http://localhost

This comment has been minimized.

@wochinge

wochinge Oct 19, 2018

Contributor

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

@wochinge

This comment has been minimized.

Copy link
Contributor

wochinge commented Oct 19, 2018

Don't worry about updating the documentation by the way, I just created a pull request for the documentation on this great feature (#1198) 🚀

@souvikg10

This comment has been minimized.

Copy link
Contributor

souvikg10 commented Oct 25, 2018

I created a new method that checks if attributes are present from Endpoint, if not sets to None so tracker_store gets it as default. except for URL, which is mandatory. I did this in tracker_store.py making sure it is agnostic to any tracker store. However if new tracker stores are added which needs more params, then defaults needs to be set. Not sure how to avoid that

store.timeout = None
return store

def find_tracker_store(self, store=None):

This comment has been minimized.

@wochinge

wochinge Oct 26, 2018

Contributor

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.

This comment has been minimized.

@souvikg10

souvikg10 Oct 26, 2018

Contributor

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

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)]

This comment has been minimized.

@wochinge

wochinge Oct 26, 2018

Contributor

If we replace these lines with

self.store_type = kwargs.pop('store_type', None)
self.kwargs = kwargs

then we can simplify this whole default parameter thing -> see find_tracker_store for the follow up

This comment has been minimized.

@souvikg10

souvikg10 Oct 26, 2018

Contributor

won't you need this for all the args for the tracker store and set them to None?

This comment has been minimized.

@souvikg10

souvikg10 Oct 26, 2018

Contributor

okay, i see the other review. It will automatically be passed to the Redis or Mongo Tracker

This comment has been minimized.

@wochinge

wochinge Oct 26, 2018

Contributor

If we pass the extra tracker store parameters as dictionary and then pass the dictionary as key word arguments (**kwargs) to the actual constructor of a tracker store, then the actual tracker store should provide the default values. Was that understandable?

This comment has been minimized.

@wochinge

wochinge Oct 26, 2018

Contributor

Ah, you were faster 👍

Show resolved Hide resolved rasa_core/tracker_store.py Outdated
@wochinge

This comment has been minimized.

Copy link
Contributor

wochinge commented Oct 26, 2018

@souvikg10 The suggested approach is open for discussion. It's just what I came up with when I read your previous comment.

@souvikg10

This comment has been minimized.

Copy link
Contributor

souvikg10 commented Oct 26, 2018

your approach looks cleaner. I was wondering this yesterday about what happens if there is a new custom tracker_store which is linked to your enhancement request regarding adding any tracker store in the future

@wochinge

This comment has been minimized.

Copy link
Contributor

wochinge commented Oct 26, 2018

Yes, that's exactly the same I thought. If got this great possibility to define everything in the endpoints.yml then it would be quite cumbersome to have to adjust the check_attributes all the time and set values to None we might not even need.

Show resolved Hide resolved rasa_core/tracker_store.py Outdated
Show resolved Hide resolved rasa_core/run.py Outdated
Show resolved Hide resolved rasa_core/train.py Outdated

@amn41 amn41 referenced this pull request Oct 26, 2018

Closed

release core 0.12 #1230

@@ -245,10 +246,13 @@ 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"))

This comment has been minimized.

@tmbo

tmbo Oct 29, 2018

Member

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...

Show resolved Hide resolved rasa_core/train.py Outdated
@tmbo

This comment has been minimized.

Copy link
Member

tmbo commented Oct 29, 2018

I've added a comment about the domain loading, wich we need to remove. Otherwise it will fail for agents stored on remote servers.

But other than that it is a 🚀 from my side. Thanks a lot for putting so much effort into this @souvikg10

@tmbo

tmbo approved these changes Oct 30, 2018

@MetcalfeTom
Copy link
Contributor

MetcalfeTom left a comment

Really appreciate the work you've put into this @souvikg10. The functionality is great and the code is almost there but I would just like to see if we can pass the codeclimate check.

Other than that, ready to merge

Show resolved Hide resolved rasa_core/tracker_store.py
Show resolved Hide resolved rasa_core/tracker_store.py Outdated

souvikg10 added some commits Oct 30, 2018

@wochinge

This comment has been minimized.

Copy link
Contributor

wochinge commented Oct 31, 2018

@MetcalfeTom then we are ready to merge?

@MetcalfeTom MetcalfeTom merged commit 770aa86 into RasaHQ:master Oct 31, 2018

3 checks passed

codeclimate All good!
Details
coverage/coveralls Coverage increased (+0.003%) to 78.758%
Details
license/cla Contributor License Agreement is signed.
Details
@MetcalfeTom

This comment has been minimized.

Copy link
Contributor

MetcalfeTom commented Oct 31, 2018

Congratulations on another successful merge @souvikg10

@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