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

Adds support for dynamic entity value matching #205

Closed
wants to merge 1 commit into from

Conversation

rozele
Copy link
Contributor

@rozele rozele commented Oct 4, 2019

In order to support matching of dynamic entity values, this commit introduces to changes.

First, it adds a context property to each predicted utterance from the NLU service, including a timestamp value for when the prediction request was made.

Second, it adds a mechanism to include inline scripts, which are evaluated at runtime with Roslyn.

Fixes #204

}
],
"intent": "None",
"text": "what's on my calendar tomorrow"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When I run something similar to this against dialogflow. The compare is failing with "Actual utterance does not have entity matching 'tomorrow"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still have an issue with time. DF returns something like 2019-10-05T12:00:00-04:00 and what we get here is 2019-10-05T00:00:00Z so the comparison fails


In reply to: 331507665 [](ancestors = 331507665)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, actually, it shouldn't be a problem. Presumably the same client that ran the DF request will also run the compare command, so it will have the same timezone. You just need to account for how DF considers the default time to be noon instead of midnight.

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.e., your script would look like:

csharp(DateTimeOffset.Now.AddDays(1).AddHours(12).ToString("yyyy-MM-ddTHH:mm:sszzz")

@saroup
Copy link
Contributor

saroup commented Oct 4, 2019

I am not fully convinced of this solution yet and here is why:
1- let's say we have a test set that contains things like tomorrow and next Friday. We need to have an additional external component that goes over the date entities and parse those into csharp date.
2- If there is some error in the csharp code in json we would only figure that out when after running the tests against the NLU provider when we run the compare method and it feels expensive as a process.

We can have a built in date parser that looks for entity type dates and converts those into datetime. This way the user building the test set doesn't have to worry about parsing these and converting them in the test set.

@rozele
Copy link
Contributor Author

rozele commented Oct 4, 2019

@saroup - how do we specify how the two date objects would be compared?

@rozele
Copy link
Contributor Author

rozele commented Oct 4, 2019

@saroup

  1. I don't understand why we need an external component, we just need people to imperatively specify (in code) what they intend by "tomorrow" or "next Friday". That is the point of this code.
  2. The error is detected at the point we run compare. At this point, we already have run (and saved) the NLU test results, so we only need to re-run the compare command to fix any bugs we have in our dynamic entity resolution logic.

@rozele
Copy link
Contributor Author

rozele commented Oct 7, 2019

I think we should extract the context / timestamp logic from this and put the rest on hold for now.

In order to support matching of dynamic entity values, this commit introduces to changes.

First, it adds a `context` property to each predicted utterance from the NLU service, including a `timestamp` value for when the prediction request was made.

Second, it adds a mechanism to include inline scripts, which are evaluated at runtime with Roslyn.

Fixes microsoft#204
@rozele
Copy link
Contributor Author

rozele commented Oct 10, 2019

Closing PR in favor of #206 for now.

@rozele rozele closed this Oct 10, 2019
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.

Allow scripting to support dynamic entity value matches
2 participants