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 experimental use of LUIS batch testing #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rozele
Copy link
Owner

@rozele rozele commented Nov 5, 2020

Adds a new interface for NLU providers capable of providing batch testing capabilities. Creates an implementation of the batch testing interface for LUIS.

Fixes microsoft#334

Adds a new interface for NLU providers capable of providing batch testing capabilities. Creates an implementation of the batch testing interface for LUIS.

Fixes microsoft#334
Copy link

@in4margaret in4margaret left a comment

Choose a reason for hiding this comment

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

Did you mean to create a PR to your fork?

docs/LuisEndpointConfiguration.md Outdated Show resolved Hide resolved
docs/LuisEndpointConfiguration.md Show resolved Hide resolved
var delta = 1;
var retryAfter = DateTimeOffset.Now.AddSeconds(10).ToString("r", CultureInfo.InvariantCulture);
var delay = Retry.GetRetryAfterDelay(retryAfter, TimeSpan.Zero);
delay.TotalSeconds.Should().BeApproximately(10, delta);

Choose a reason for hiding this comment

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

Why do we use delta=1 here, but in previous test it is value 10e-6.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I actually should not use BeApproximately here, because it's +/- delta. The problem with this test is that the implementation of GetRetryAfterDelay uses DateTimeOffset.Now, rather than some clock interface that can be easily mocked. So, this test is technically non-deterministic because we don't know how much of a diff will be between our estimated clock time (on L31) and the clock time in the GetRetryAfterDelay implementation, though given this is a synchronous operation without much compute work, we can be pretty confident that it will be < O(100ms). Thus I chose 1 second as a possible diff value.

It's a good catch though, as this tests 9 <= delay <= 11 (due to BeApproximately) when what we really want is 9 <= delay <= 10.

var matchIndex = jsonObject.Value<int>("matchIndex");

// Remove the generic entity properties
jsonObject.Remove("entityType");

Choose a reason for hiding this comment

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

Should we modify in place or create a new object with fields that we need to have good understanding of what is required?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We modify in place because the batch API may support additional properties that we don't care about. For example, if the batch API started allowing users to specify a role property on each entity, we should pass that through. The only thing we're trying to do is remove any of the generic utterance format properties (matchText, matchIndex and entityType) and replace them with LUIS batch testing format. It's safe to modify in place because we do a deep clone on L129.

@rozele
Copy link
Owner Author

rozele commented Nov 12, 2020

Did you mean to create a PR to your fork?

@in4margaret - yes, this won't be ready for merge into the main repo for a while.

- Fixes issue with Retry test.
- Adds test for batch operator where item count modulo batch size is non-zero.
- Adds documentation to note that speech and batch testing are incompatible.
- Fixes typo in documentation.
@rozele rozele force-pushed the issue334 branch 2 times, most recently from 15f1ceb to 00e5929 Compare November 12, 2020 15:11
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.

Add interface and update test command for batch testing
2 participants