Domain API endpoint #679

JoeyFaulkner · 2018-06-29T12:46:46Z

Proposed changes:

create domain from yaml string
create yaml string from domain
expose endpoint for fetching domain yaml

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

amn41

think it's OK - but maybe we should think about persisting domains in saved models as yaml as well then rather than converting to json

amn41 · 2018-06-29T13:14:27Z

tests/test_domain.py

+  utter_greet:
+  - text: hey there!"""
+    domain = TemplateDomain.load_from_yaml(test_yaml)
+    assert test_yaml in domain.to_yaml()


why 'in' ? would x.strip() == y.strip() also work?

yep, that works fine!

amn41 · 2018-06-29T13:15:37Z

rasa_core/utils.py

@@ -17,6 +17,10 @@
 from builtins import input, range, str
 from numpy import all, array
 from typing import Text, Any, List, Optional, Tuple, Dict, Set
+try:


is this a py2 / py3 thing? can we use six instead?

it is, use six how?

saw your other comment, ignore me!

amn41 · 2018-06-29T13:15:50Z

rasa_core/server.py

+    @requires_auth(auth_token)
+    @ensure_loaded_agent(agent)
+    def get_domain_yaml():
+        """Use a list of events to set a conversations tracker to a state."""


description needs updated

amn41 · 2018-06-29T13:18:01Z

rasa_core/domain.py

+            action_factory = data.get("action_factory", None)
+        slots = cls.collect_slots(data.get("slots", {}))
+        additional_arguments = data.get("config", {})
+        return TemplateDomain(


why do we have both cls and TemplateDomain in this method? surely they are the same? if we are allowing for subclassing then probably stick with cls ?

oh yes, good call

amn41 · 2018-06-29T13:19:19Z

rasa_core/utils.py

@@ -319,6 +323,21 @@ def read_yaml_file(filename):

        return yaml_parser.load(read_file(filename))

+def read_yaml_string(string):


can we then rewrite read_yaml_file to just be read_yaml_string(read_file(filename)) ?

amn41 · 2018-06-29T13:19:37Z

rasa_core/utils.py

@@ -319,6 +323,21 @@ def read_yaml_file(filename):

        return yaml_parser.load(read_file(filename))

+def read_yaml_string(string):
+
+    if six.PY2:


here an example of using six

tmbo · 2018-06-29T16:45:50Z

rasa_core/domain.py

@@ -416,7 +416,27 @@ def load(cls, filename, action_factory=None):
        )

    @classmethod
-    def validate_domain_yaml(cls, filename):
+    def load_from_yaml(cls, yaml, action_factory=None):


can load use load_from_yaml once it read the yaml from the file as well? want to avoid the duplication

tmbo · 2018-06-29T16:52:38Z

rasa_core/utils.py

+def dump_obj_as_yaml_to_string(obj):
+    """Writes data (python dict) to a yaml string."""
+
+    if six.PY2:


these two functions should share the code, eg:

def _dump_yaml(obj, output): #... yaml_writer.dump(obj, output) #... def dump_obj_as_yaml_to_file(filename, obj): with io.open(filename, 'w', encoding="utf-8") as yaml_file: _dump_yaml(obj, yaml_file) def dump_obj_as_yaml(obj): str_io = StringIO() _dump_yaml(obj, str_io) return str_io.getvalue()

tmbo · 2018-06-29T16:54:14Z

rasa_core/server.py

+    @ensure_loaded_agent(agent)
+    def get_domain_yaml():
+        """Get current domain in yaml format."""
+        domain_yaml = agent().domain.to_yaml()


why are we sending the domain as yaml instead of json?

I mean its the way that the domain is stored in the model folder so why not?

this is a REST api, so usually the responses are in json format. I mean I don't have a strong opinion on this, but if yaml is used, content negotiation should be used: e.g.

accepts = request.headers.get("Accept", default="application/json") if accepts.endswith("json"): domain = agent().domain.as_dict() return jsonify(domain) elif accepts.endswith("yml"): domain = agent().domain.as_yaml() return Response(domain_yaml, status=200, content_type="application/x-yml") else: return Response("""Invalid accept header. Domain can be provided as json ("Accept: application/json") or yml ("Accept: application/x-yml"). Make sure you've set the appropriate Accept header.""", status=406)

tmbo · 2018-07-02T09:14:24Z

rasa_core/domain.py

        )

    @classmethod
-    def validate_domain_yaml(cls, filename):
+    def validate_domain_yaml(cls, input, string_input=False):


input needs to be renamed (shadows build in function)

do we need string_input? I think it is always True

it wasnt in tests but i changed tests :)

tmbo · 2018-07-02T09:18:58Z

rasa_core/domain.py

@@ -496,7 +504,7 @@ def instantiate_actions(factory_name, action_classes, action_names,
    def _slot_definitions(self):
        return {slot.name: slot.persistence_info() for slot in self.slots}

-    def persist(self, filename):
+    def _make_domain_data(self):


def as_dict(self) (for consistency across classes)

tmbo · 2018-07-02T09:20:12Z

rasa_core/domain.py

        utils.dump_obj_as_yaml_to_file(filename, domain_data)

+    def to_yaml(self):


as_yaml for consistency

tmbo · 2018-07-02T09:44:57Z

rasa_core/server.py

+    @ensure_loaded_agent(agent)
+    def get_domain_yaml():
+        """Get current domain in yaml format."""
+        domain_yaml = agent().domain.to_yaml()


this is a REST api, so usually the responses are in json format. I mean I don't have a strong opinion on this, but if yaml is used, content negotiation should be used: e.g.

accepts = request.headers.get("Accept", default="application/json") if accepts.endswith("json"): domain = agent().domain.as_dict() return jsonify(domain) elif accepts.endswith("yml"): domain = agent().domain.as_yaml() return Response(domain_yaml, status=200, content_type="application/x-yml") else: return Response("""Invalid accept header. Domain can be provided as json ("Accept: application/json") or yml ("Accept: application/x-yml"). Make sure you've set the appropriate Accept header.""", status=406)

tmbo

small change left that needs to be done. also, please make sure to add your change to CHANGELOG.rst

tmbo · 2018-07-02T13:59:33Z

rasa_core/domain.py

        )

    @classmethod
-    def validate_domain_yaml(cls, filename):
+    def validate_domain_yaml(cls, input):


still needs to be renamed to something that is not input (shadows built in)

JoeyFaulkner added 2 commits June 29, 2018 13:42

domain yaml endpoint

2f650be

added tests

73ca75c

JoeyFaulkner requested review from tmbo and amn41 June 29, 2018 12:47

JoeyFaulkner added 2 commits June 29, 2018 14:48

small fix

df88903

rogue underscore

3cc7047

amn41 reviewed Jun 29, 2018

View reviewed changes

JoeyFaulkner added 2 commits June 29, 2018 15:32

@amn41 fixes

114af62

small fix

4d3ae82

tmbo suggested changes Jun 29, 2018

View reviewed changes

JoeyFaulkner added 2 commits July 2, 2018 09:37

functions share code

fea258e

whoops

8bb98b4

tmbo suggested changes Jul 2, 2018

View reviewed changes

JoeyFaulkner added 2 commits July 2, 2018 13:32

@tmbo fixes

949356a

smol fix

7742b3e

tmbo approved these changes Jul 2, 2018

View reviewed changes

@tmbo fixes

66265cd

JoeyFaulkner merged commit 5eae121 into master Jul 5, 2018

JoeyFaulkner deleted the domain_endpoint branch July 5, 2018 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain API endpoint #679

Domain API endpoint #679

JoeyFaulkner commented Jun 29, 2018 •

edited

amn41 left a comment

amn41 Jun 29, 2018

JoeyFaulkner Jun 29, 2018

amn41 Jun 29, 2018

JoeyFaulkner Jun 29, 2018

JoeyFaulkner Jun 29, 2018

amn41 Jun 29, 2018

amn41 Jun 29, 2018

JoeyFaulkner Jun 29, 2018

amn41 Jun 29, 2018

amn41 Jun 29, 2018

tmbo Jun 29, 2018

tmbo Jun 29, 2018

tmbo Jun 29, 2018

JoeyFaulkner Jul 2, 2018

tmbo Jul 2, 2018

JoeyFaulkner Jul 2, 2018

tmbo Jul 2, 2018

JoeyFaulkner Jul 2, 2018

tmbo Jul 2, 2018

tmbo Jul 2, 2018

tmbo Jul 2, 2018

tmbo left a comment

tmbo Jul 2, 2018

		@@ -319,6 +323,21 @@ def read_yaml_file(filename):

		return yaml_parser.load(read_file(filename))

		def read_yaml_string(string):

		utils.dump_obj_as_yaml_to_file(filename, domain_data)

		def to_yaml(self):

Domain API endpoint #679

Domain API endpoint #679

Conversation

JoeyFaulkner commented Jun 29, 2018 • edited

amn41 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeyFaulkner commented Jun 29, 2018 •

edited