support reference_time for duckling #395
Conversation
|
The emulator tests are failing. I shall make the corrections. |
There was a problem hiding this comment.
Thanks a lot! I think even though duckling doesn't support half-hour offsets, it's still useful. (We might even be fine by passing in UTC times - not sure though?)
I am undecided yet on the input to parse. Should that be a time string (as it is now) or rather be a timestamp in milliseconds (better interoperability over http?). Whats your opinion?
_pytest/test_components.py
Outdated
| @@ -85,7 +85,7 @@ def test_all_arguments_can_be_satisfied_during_parse(component_class, default_co | |||
|
|
|||
| # All available context arguments that will ever be generated during parse | |||
| component = component_builder.create_component(component_class.name, default_config) | |||
| context_arguments = {"text": None} | |||
| context_arguments = {"text": None, "ref_time": ""} | |||
There was a problem hiding this comment.
Also, I'd like to rename this to something that expresses that this is the time of the message. Do you have a suggestion here, maybe just time?
There was a problem hiding this comment.
time should be fine
rasa_nlu/data_router.py
Outdated
| @@ -175,7 +175,7 @@ def parse(self, data): | |||
| raise InvalidModelError("No model found with alias '{}'. Error: {}".format(alias, e)) | |||
|
|
|||
| model = self.model_store[alias] | |||
| response = model.parse(data['text']) | |||
| response = model.parse(data['text'], data['ref_time']) | |||
There was a problem hiding this comment.
Should be optional - eg. data.get('time')
| @@ -77,12 +78,28 @@ def pipeline_init(self, language): | |||
| except ValueError as e: # pragma: no cover | |||
| raise Exception("Duckling error. {}".format(e)) | |||
|
|
|||
| def process(self, text, entities): | |||
| def process(self, text, entities, ref_time=""): | |||
| # check if ref_time is a datetime is in a known format | ||
| # If its not parseable, make it empty | ||
| if not ref_time: | ||
| ref_time = "" |
There was a problem hiding this comment.
How about setting it to the current server time?
There was a problem hiding this comment.
Duckling takes the current system time and timezone by default.
There was a problem hiding this comment.
Right, but we might use that time for other things in the future (e.g. for logging)
|
The usefulness of this feature is that the duckling can parse the input string by using the desired context (time with timezone) and we get the exact timestamp as the parsed output. I think we should pass the timezone info somehow for parsing. If we just pass the timestamp in milliseconds, the zone info is lost. We can also take timezone info separately! |
|
Another way to standardise would be to take just |
|
Alright, lets use a UTC ISO time string then. 👍 |
|
Sorry @vinvinod I totally forgot about this one. If you don't mind fixing the merge conflicts with master, I could merge it afterwards. |
| @@ -78,12 +80,34 @@ def cache_key(cls, model_metadata): | |||
|
|
|||
| return cls.name + "-" + model_metadata.language | |||
|
|
|||
| def process(self, text, entities): | |||
| def pipeline_init(self, language): | |||
There was a problem hiding this comment.
This function needs to be removed (I don't think it is necessary anyways), it most likely is an artifact from resolving the merge conflicts with master. This function is also the reason the tests fail.
vinvinod
force-pushed
the
duckling_ref_time
branch
from
afb942a
to
39d143c
Compare
|
nice work 😃 👍 |
|
👍 |
Proposed changes:
Duckling module also supports passing a context using reference_time argument. We can pass a string in RFC3339 format (Eg: '2008-09-08T22:47:31-07:00').
Example: Parsing "tomorrow" with reference_time set to
2008-09-08T20:30:00-07:00gives back2008-09-09T00:00:00.000-07:00Since Rasa supports duckling in the pipeline, I think this will be a useful addition.
Status: