Skip to content
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

support reference_time for duckling #395

Merged
merged 6 commits into from
Jun 19, 2017
Merged

Conversation

vinvinod
Copy link
Contributor

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:00 gives back
2008-09-09T00:00:00.000-07:00

Since Rasa supports duckling in the pipeline, I think this will be a useful addition.

Status:

  • ready for code review
  • there are tests for the functionality
  • documentation updated
  • changelog updated

@vinvinod
Copy link
Contributor Author

The emulator tests are failing. I shall make the corrections.
But I noticed thats there is an issue with duckling. It doesnt handle timezone offsets with non-zero minutes (facebookarchive/duckling_old#209)
So will this feature be useful?

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

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?

@@ -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": ""}
Copy link
Member

Choose a reason for hiding this comment

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

Let's use None as the default

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time should be fine

@@ -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'])
Copy link
Member

Choose a reason for hiding this comment

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

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=""):
Copy link
Member

Choose a reason for hiding this comment

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

None instead of ""

# 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 = ""
Copy link
Member

Choose a reason for hiding this comment

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

How about setting it to the current server time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duckling takes the current system time and timezone by default.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but we might use that time for other things in the future (e.g. for logging)

@vinvinod
Copy link
Contributor Author

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!
time for time in milliseconds and timezone for taking the offset in minutes like the below request.
http://localhost:5000/parse?text=Call%20me%20tomorrow%20morning%20at%208&time=1496055692128&timezone=-330
Then we also have to validate the timezone offset value.

@vinvinod
Copy link
Contributor Author

Another way to standardise would be to take just time in millis and always assume timezone as UTC. So that the users can convert the parsed time to whatever zone they want in their applications.

@tmbo
Copy link
Member

tmbo commented May 30, 2017

Alright, lets use a UTC ISO time string then. 👍

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.549% when pulling 4db7ec4 on verloop:duckling_ref_time into 48a3dec on RasaHQ:master.

@tmbo
Copy link
Member

tmbo commented Jun 13, 2017

Sorry @vinvinod I totally forgot about this one. If you don't mind fixing the merge conflicts with master, I could merge it afterwards.

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

one issue from the merge remaining

@@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

@tmbo
Copy link
Member

tmbo commented Jun 19, 2017

nice work 😃 👍

@tmbo tmbo merged commit 03262b1 into RasaHQ:master Jun 19, 2017
@vinvinod
Copy link
Contributor Author

👍

taytzehao pushed a commit to taytzehao/rasa that referenced this pull request Jul 14, 2023
vcidst pushed a commit that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants